Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Implement rpc interface for setting external GUI features #431

Merged
merged 1 commit into from
Jul 31, 2018
Merged

[RFC] Implement rpc interface for setting external GUI features #431

merged 1 commit into from
Jul 31, 2018

Conversation

BoltsJ
Copy link
Contributor

@BoltsJ BoltsJ commented Jul 25, 2018

This will allow GUI options such as the GUI tabline, cmdline (#351), popupmenu (#349) and wildmenu to be enabled and disabled from ginit.vim instead of command line switches.

The interface will be :call rpcnotify(0, "Gui", "Option", <option_name>, <value>), the same as neovim-gtk.

Ref #361 #362 #366

TODO

  • Check to make sure appropriate api level is available
  • Use info reported by nvim to control status
  • Hide gui tabline when disabled
  • Create widget as hidden even when --no-ext-tabline is specified
  • Add tests

@BoltsJ
Copy link
Contributor Author

BoltsJ commented Jul 30, 2018

@equalsraf This should be ready for you to review.

@BoltsJ BoltsJ changed the title WIP - Implement rpc interface for setting external GUI features [RFC] Implement rpc interface for setting external GUI features Jul 30, 2018
Copy link
Owner

@equalsraf equalsraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. In terms of usability I would prefer Shell to emit specific signals for each options.

I've added some helper code for the shim and tests.

README.md Outdated
@@ -52,6 +52,10 @@ Commands for interacting with the GUI are regular commands, available in the doc

You can set GUI options on startup, in the GUI configuration file (:help ginit.vim).

To disable the GUI tabline and use the nvim TUI tabline, call

:call rpcnotify(0, "Gui", "Option", "Tabline", 0)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is worth it to add a command to the shim, instead of having the user call rpcnotify

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function s:GuiExtTabline(enable)
	call rpcnotify(0, "Gui", "Option", "Tabline", a:enable)
endfunction
command -nargs=1 GuiExtTabline call s:GuiExtTabline(<args>)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the command as GuiTabline.

@@ -45,6 +45,7 @@ private slots:
void neovimIsUnsupported();
void neovimShowtablineSet(int);
void neovimTablineUpdate(int64_t curtab, QList<Tab> tabs);
void neovimGuiOptionSet(const QString& name, const QVariant& value);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing around the QVariant from Shell to the MainWindow, I would prefer to have per option signals/slots. For example this could be

void MainWindow::extTablineSet(bool);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I changed the signal to Shell::neovimExtTablineSet and the matching slot to MainWindow::extTablineSet.

QSignalSpy onOptionSet(s, &Shell::extGuiOptionSet);
QVERIFY(onOptionSet.isValid());
QVERIFY(SPYWAIT(onOptionSet));
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add another test in guiShimCommands() that checks for the signal when using vim_command

diff --git a/test/tst_shell.cpp b/test/tst_shell.cpp
index b813494..89d583e 100644
--- a/test/tst_shell.cpp
+++ b/test/tst_shell.cpp
@@ -123,6 +123,12 @@ private slots:
                checkCommand(c, "GuiFont DejaVu Sans Mono:h14:b:l", false);
                QCOMPARE(s->shell()->fontDesc(), QString("DejaVu Sans Mono:h14:l"));
 #endif
+
+               QSignalSpy onOptionSet(s->shell(), &Shell::extGuiOptionSet);
+               checkCommand(c, "call rpcnotify(0, \"Gui\", \"Option\", \"Tabline\", 0)", false);
+               QVERIFY(onOptionSet.isValid());
+               QVERIFY(SPYWAIT(onOptionSet));
+               qDebug() << onOptionSet << onOptionSet.size();
        }
 
 protected:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added your suggested test for the GuiTabline command.

@equalsraf
Copy link
Owner

Looks good. Can you rebase into a single commit so I can merge?

@equalsraf
Copy link
Owner

I just realized that starting with --no-ext-tabline causes an empty tab bar to be visible on my system

Add handler for GUI option rpc notifications. Currently, only tabline is
handled.

Add signal for when nvim updates the value of ext_tabline and a matching
slot to control visibility of the gui tabline.

Add GuiTabline ex command to nvim_gui_shim.vim to enable and disable the GUI
tabline. Document in runtime/doc/nvim_gui_shim.txt and README.md.
@BoltsJ
Copy link
Contributor Author

BoltsJ commented Jul 31, 2018

I fixed the empty tab bar and squashed the commits. Everything should be good to go.

@equalsraf equalsraf merged commit a250faf into equalsraf:master Jul 31, 2018
@BoltsJ BoltsJ deleted the tb-rpcoptions branch July 31, 2018 21:05
@BoltsJ BoltsJ mentioned this pull request Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants