Client systray support #565

townsend2010 · 2019-01-18T21:14:06Z

Add a client systray icon and menu. This allows:

Getting a list of all instances and state.
The ability to open a shell to an instance.
Stopping an instance.
Getting version information.
Displaying when an update to Multipass is available.

townsend2010 · 2019-01-18T21:37:38Z

To @gerboland's comments in #564, I wonder if the main event loop I added is indeed unnecessary.

My question is if we can use a local QEventLoop in Systray::run() and Qt will be happy processing events? If so, then the first commit in this can be removed. I think I'll experiment with that.

gerboland · 2019-01-22T14:39:22Z

src/client/main.cpp

@@ -48,7 +48,7 @@ std::string get_server_address()

 int main(int argc, char* argv[])
 {
-    QCoreApplication app(argc, argv);
+    QApplication app(argc, argv);


Hmm, can you check if the command line runs when no display server is running (i.e. "DISPLAY=")

QApplication subclasses QGuiApplication, which loads Qt platform backends for various graphical shells. Those are needed for the systray icon ofc, but for a pure command-line util, I'm unsure if it'll run or just bail.

No, it doesn't:( Offhand, do you if there is some way to load the platfrom backends after the fact, like when only the systray command runs? I'll do some research, but hoping you have a quick answer:)

Backends load on Q{Gui,}Application construction.

One workaround might be to read the CLI very early looking for systray:

if (!argv_has_systray()) setenv("QT_QPA_PLATFORM", "minimal"); QApplication app(argc, argv);

That did the trick. Thanks!

Wouldn't it be better to have a separate command altogether then? Just a separate main instead of the if?

@ricab,
I want to use the same binary, ie, multipass for the cli, systray icon, and the down-the-road full blown gui, so I think the simple if will do the trick.

townsend2010 · 2019-02-01T17:20:12Z

BTW, the design of this is based closely on https://github.com/CanonicalLtd/multipass-design#status-menu with added changes as I saw necessary.

ricab

Detail: systray missing autocompletion

ricab · 2019-02-05T11:53:28Z

OK, so I finally managed to build the snap and launch multipass from it... but crash again :/

$ multipass systray
This application failed to start because it could not find or load the Qt platform plugin "xcb"
in "".

Available platform plugins are: eglfs, linuxfb, minimal, minimalegl, offscreen, vnc, xcb.

Reinstalling the application may fix this problem.
Aborted (core dumped)

gdb /snap/multipass/x1/bin/multipass core says.

Some more research here and here does not help much...

ricab · 2019-02-05T11:58:44Z

OK, the issue seems to be a missing "xinerama" library. Here is the output from Qt itself with QT_DEBUG_PLUGINS=1 (see towards the end). So we just need a new multipass dependency I guess...

Saviq

Tested on Ubuntu:

When there's no instances, there's a stray blank section:

It seems to disappear during the transition when the menu is closed. Maybe it's the separator?

I wouldn't put the instance status verbatim in the menu:

If there's no way to add the status like designed (right-aligned and greyed out), maybe we could add some Unicode trickery. We may need to go platform-dependent on this? Doesn't have to be this PR of course.

Saviq · 2019-02-05T11:59:16Z

src/platform/client/client_platform_linux.cpp

+
+void mcp::open_multipass_shell(const QString& instance_name)
+{
+    QProcess::startDetached("xterm", {"-e", "multipass", "shell", instance_name});


There's no way to detect whether the command being run in xterm failed (in case the shell command failed for some reason), in which case there's zero feedback… Maybe add || read to the command being run?

Right, I had planned on giving extra feedback when the terminal cannot be started for whatever reason. Ideally, I'd like it to pop up a window with the failure and reason, but for a short term solution, your idea will work.

Let's avoid windows whenever possible to keep the experience coherent. Multipass is anchored in command line usage so it shouldn't be surprising that we don't have many dialogs and windows. Those are always focus hogs.

src/platform/client/client_platform_linux.cpp

townsend2010 · 2019-02-05T13:07:51Z

I wouldn't put the instance status verbatim in the menu

Right, I have plans to fix that. I do think we should be consistent with the output of multipass list though, so we should fix it there and then carry it over here.

It seems to disappear during the transition when the menu is closed. Maybe it's the separator?

Yeah, I'm sure it's the separator and I missed that. Will fix.

If there's no way to add the status like designed (right-aligned and greyed out), maybe we could add some Unicode trickery.

I think this is going to be real tricky. I spent some time trying to make it work like the design with no luck. I may end up having to subclass QMenu and try some things in that in forcing Qt to display it how we want to.

townsend2010 · 2019-02-05T13:10:05Z

Detail: systray missing autocompletion

Ack, will add that to the autocompletions.

OK, the issue seems to be a missing "xinerama" library.

I think once we switch the snap to use base: core18, then I can use the desktop-qt5 remote part and that should pull in all of the dependencies and such that we need.

Saviq · 2019-02-05T14:20:07Z

clang on Mac says:

../src/client/cmd/systray.cpp:105:77: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
            QObject::connect(instance_actions.back(), &QAction::triggered, [this, name] {
                                                                            ^
../src/client/cmd/systray.cpp:178:24: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
    auto on_success = [this, &list_reply](ListReply& reply) {
                       ^
2 errors generated.

townsend2010 · 2019-02-05T14:58:36Z

@Saviq,

When there's no instances, there's a stray blank section:

Would it be better to display something like "No instances" or "No instances defined" or something when there are no instances? When it just has 'About' and 'Quit' there, it kind of looks like it's missing something.

Saviq · 2019-02-05T15:21:42Z

@townsend2010 there ultimately will be a Launch, won't there? And the whole pet story…

townsend2010 · 2019-02-05T15:28:31Z

@Saviq,

I'm not under the impression that Launch will be part of the systray drop down menu. I don't see that in the design of the systray icon either. I do think Launch would be part of the main UI though.

Regarding the pet story, the only thing we would add is a specific action with a hotkey attached to it for opening the shell.

Saviq · 2019-02-05T15:35:12Z

@townsend2010 right, but that means there will always be an item above About - and there will never be "No instances" in that sense.

ricab · 2019-02-06T11:01:58Z

Some things that I noticed (but could belong in polishing PRs):

The contents of the menu are sometimes updated, sometimes not:
- when clicking "start" on an instance and then clicking again to monitor its state: it will go from STOPPED to STARTING, but will never update to RUNNING in the menu. So STOPPED-->STARTING-/->RUNNING. The RUNNING state is only shown after clicking outside and clicking the widget again.
- the same happens when clicking suspend: RUNNING-->SUSPENDING-/->SUSPENDED
- I tried sending commands from the terminal and immediately clicking on the icon. The same thing happens.
  - Clicking it right after launch, no new instance appears (until click out and in)
- stop seems to be good: RUNNING-->STOPPED directly
When the contents of the menu are updated, it briefly flickers out-of and into existence. It would be nice if the transition could be smooth.

Saviq · 2019-02-06T11:52:19Z

When the contents of the menu are updated, it briefly flickers out-of and into existence. It would be nice if the transition could be smooth.

What desktop environment is this? I don't think we can influence how they deal with this (unless in fact we're deleting the whole menu and recreating it).

ricab · 2019-02-06T12:01:28Z

Regarding:

$ multipass systray
QStandardPaths: XDG_RUNTIME_DIR points to non-existing path '/run/user/1000/snap.multipass', please create it with 0700 permissions.

This says:

$XDG_RUNTIME_DIR defines the base directory relative to which user-specific non-essential runtime files and other file objects (such as sockets, named pipes, ...) should be stored. The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700.

I noticed it is mentioned here and from some quick googling I got the impression it is relevant for security concerns.

ricab · 2019-02-06T12:02:25Z

@Saviq Unity on 16.04 here. Do you see something else?

ricab · 2019-02-06T12:04:18Z

@Saviq other similar menus update continuously here (e.g. system monitor)

townsend2010 · 2019-02-06T14:12:10Z

@ricab,

Thanks for the feedback. In general, there is much work to be done on this:) But to reply to your feedback:

The contents of the menu are sometimes updated, sometimes not:

Yes, there is absolutely no polling or event handling when the menu has been opened. Currently, the only way to refresh the states is to close the menu and then reopen it. Polling will be implemented first and then eventually some event driven model.

When the contents of the menu are updated, it briefly flickers out-of and into existence. It would be nice if the transition could be smooth.

Right, this will be fixed. But what you are seeing is the static menu from the last query, ie, the last time you opened the menu, and then once the client receives the list info from the daemon, it clears the menu and then creates a new one based on what it received.

gerboland

This is much cleaner, I like this. I've a couple of small comments inline.

Nitpick: there's many files in src/client/cmd that are getting solely formatting changes, making the diff a lot bigger than it needs to be. Has clang-format changed its mind about some things?

src/client/gui/gui_cmd.cpp

src/client/gui/main.cpp

src/client/gui/argparser.h

townsend2010 · 2019-02-08T14:56:55Z

@gerboland,

Thanks for the review. Glad you like this iteration better. Now that I put in the hard work, I think it's better too:)

Nitpick: there's many files in src/client/cmd that are getting solely formatting changes, making the diff a lot bigger than it needs to be. Has clang-format changed its mind about some things?

Yes, silly clang-format decided to change a bunch of things and I really didn't want to have to keep backing those out. Not sure why it decided to change now, but it did:/

- Use status enum instead of strings for status comparisons. - Remove "Retrieving instances" message which makes "About" happy when there are no instances. - Fix clang copy elision error.

gerboland · 2019-05-13T10:19:47Z

"About" issue fixed.

New issue: when VM in "Stopped" state, I am shown useless "Open Shell" option. I cannot restart VM from the UI.

Saviq · 2019-05-13T12:38:57Z

New issue: when VM in "Stopped" state, I am shown useless "Open Shell" option.

It shouldn't be useless, it should start the instance when invoked.

I cannot restart VM from the UI.

It would be better to replace with Start, when stopped, indeed.

gerboland · 2019-05-13T13:08:33Z

New issue: when VM in "Stopped" state, I am shown useless "Open Shell" option.

It shouldn't be useless, it should start the instance when invoked.

Maybe so, but that's not obvious from the UI.

townsend2010 · 2019-05-13T13:20:06Z

Regarding Open shell, it's analogous to multipass shell foo, which will automatically start the instance if it's not running and then once running, it will connect. This is no different and we all agreed that is the behavior we wanted. However, I'm not really sure we really want to force the user to go through 2 separate steps in the Ui to open the shell, ie, an explicit Start, then make them wait and then an explicit Open shell.

Maybe if the instance is stopped, then it says Start & open shell there? And if it's already running, it just says, Open shell like now?

gerboland · 2019-05-13T13:21:23Z

I like that, it tells you what it will do.

Saviq · 2019-05-13T13:42:01Z

then it says Start & open shell there?

A bit verbose, but OK…

townsend2010 · 2019-05-13T13:50:31Z

A bit verbose, but OK…

I agree, but not sure what else to do since having an active action is not enough to signal to the user they can do something.

Saviq · 2019-05-13T13:51:44Z

I agree, but not sure what else to do since having an active action is not enough to signal to the user they can do something.

In a well-designed UI, isn't it? ;)

townsend2010 · 2019-05-13T13:56:49Z

In a well-designed UI, isn't it? ;)

Not sure what you mean? I thought it was clear that if the Open shell action is active, ie, clickable, that a user would click on it if the instance is even stopped and then they would see the terminal open with start status and then the shell is connected once the start succeeds.

But @gerboland disagrees and he's a UI guy, so not sure what else to do except say Start & open shell. Otherwise, we make the user do two explicit steps.

ricab · 2019-05-13T14:29:16Z

In this case, I think I'd go with open shell only. I normally prefer explicit stuff, but Start & open shell seems verbose for a UI button.

I understand it may feel unclear at first, because start is also offered to the user separately. But, as @townsend2010 pointed out, shell already starts in the CLI. In both cases, I think it is reasonable to view shell opening as an end goal that may require implicit intermediary steps. And client consistency has some non-negligable value IMO. Just my two cents.

ricab · 2019-05-13T14:32:36Z

I just went to check the design for this. Why not use "Connect" as described there?

townsend2010 · 2019-05-13T14:34:44Z

I just went to check the design for this. Why not use "Connect" as described there?

Because the "Connect" verbiage has been deprecated in favor of "Shell" like in the CLI and the design doc was never updated...at least that is how I took it when we switched the CLI from using connect to shell for this.

ricab · 2019-05-13T14:36:56Z

@townsend2010 Ah ok, fair enough. I have the feeling you had said so already? Sorry if that was the case, I did not memorize that history bit.

townsend2010 · 2019-05-13T14:38:42Z

@ricab, I don't think I mentioned that already:) Just getting you up to speed and also leaving it open a bit that "Connect" in the GUI is still what is wanted. Things change and move yet docs aren't updates, so I'm not always sure:)

CPack doesn't handle hyphens in the target name well

gerboland · 2019-05-14T14:04:55Z

Overall I'm ok with this, and we can apply any polish in later PRs
bors r+

565: Client systray support r=gerboland a=townsend2010 Add a client systray icon and menu. This allows: - Getting a list of all instances and state. - The ability to open a shell to an instance. - Stopping an instance. - Getting version information. - Displaying when an update to Multipass is available. Co-authored-by: Chris Townsend <[email protected]>

bors · 2019-05-14T14:51:41Z

Build failed

continuous-integration/travis-ci/push

townsend2010 added the no-merge label Jan 18, 2019

townsend2010 changed the title ~~Client systray support~~ WIP - Client systray support Jan 18, 2019

townsend2010 force-pushed the client-systray-support branch from 17ae950 to bdb2b82 Compare January 18, 2019 21:18

townsend2010 force-pushed the client-systray-support branch from bdb2b82 to aabc29b Compare January 22, 2019 14:26

gerboland reviewed Jan 22, 2019

View reviewed changes

townsend2010 force-pushed the client-systray-support branch from a096ae9 to 3617b87 Compare February 1, 2019 16:24

ricab reviewed Feb 4, 2019

View reviewed changes

Saviq requested changes Feb 5, 2019

View reviewed changes

gerboland reviewed Feb 8, 2019

View reviewed changes

src/client/gui/gui_cmd.cpp Outdated Show resolved Hide resolved

src/client/gui/main.cpp Outdated Show resolved Hide resolved

src/client/gui/main.cpp Outdated Show resolved Hide resolved

src/client/gui/argparser.h Outdated Show resolved Hide resolved

townsend2010 force-pushed the client-systray-support branch 2 times, most recently from a59c2a4 to 8d11394 Compare February 20, 2019 06:42

townsend2010 force-pushed the client-systray-support branch from d7229a6 to 4bfb15f Compare March 5, 2019 15:04

Chris Townsend added 3 commits May 10, 2019 12:28

gui: Support retrieving and displaying version and update available

9963a50

client: Use lower case letters in instance status strings

4f70eaa

gui: Use new Multipass icon

c9de357

townsend2010 force-pushed the client-systray-support branch from ef8afaf to 5883ff6 Compare May 10, 2019 16:34

gui: Changes based on review feedback

c35516f

- Use status enum instead of strings for status comparisons. - Remove "Retrieving instances" message which makes "About" happy when there are no instances. - Fix clang copy elision error.

townsend2010 force-pushed the client-systray-support branch from 5883ff6 to c35516f Compare May 10, 2019 19:16

gui: Add explicit "Start" action when instance is stopped

eeacb2f

Saviq force-pushed the client-systray-support branch from 5d62e92 to 7ef094d Compare May 13, 2019 19:26

[gui] rename target to multipass_gui

ccea4d9

CPack doesn't handle hyphens in the target name well

Saviq force-pushed the client-systray-support branch from 7ef094d to ccea4d9 Compare May 13, 2019 19:54

gerboland approved these changes May 14, 2019

View reviewed changes

townsend2010 merged commit ccea4d9 into master May 14, 2019

bors bot deleted the client-systray-support branch May 14, 2019 15:11

Client systray support #565

Client systray support #565

Conversation

townsend2010 commented Jan 18, 2019 • edited Loading

townsend2010 commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

townsend2010 commented Feb 1, 2019

ricab left a comment

Choose a reason for hiding this comment

ricab commented Feb 5, 2019

ricab commented Feb 5, 2019

Saviq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

townsend2010 commented Feb 5, 2019

townsend2010 commented Feb 5, 2019

Saviq commented Feb 5, 2019

townsend2010 commented Feb 5, 2019

Saviq commented Feb 5, 2019

townsend2010 commented Feb 5, 2019

Saviq commented Feb 5, 2019

ricab commented Feb 6, 2019

Saviq commented Feb 6, 2019 • edited Loading

ricab commented Feb 6, 2019

ricab commented Feb 6, 2019

ricab commented Feb 6, 2019

townsend2010 commented Feb 6, 2019

gerboland left a comment

Choose a reason for hiding this comment

townsend2010 commented Feb 8, 2019

gerboland commented May 13, 2019

Saviq commented May 13, 2019 • edited Loading

gerboland commented May 13, 2019

townsend2010 commented May 13, 2019

gerboland commented May 13, 2019

Saviq commented May 13, 2019

townsend2010 commented May 13, 2019 • edited Loading

Saviq commented May 13, 2019

townsend2010 commented May 13, 2019

ricab commented May 13, 2019

ricab commented May 13, 2019

townsend2010 commented May 13, 2019

ricab commented May 13, 2019

townsend2010 commented May 13, 2019

gerboland commented May 14, 2019

bors bot commented May 14, 2019

Build failed

townsend2010 commented Jan 18, 2019 •

edited

Loading

Saviq commented Feb 6, 2019 •

edited

Loading

Saviq commented May 13, 2019 •

edited

Loading

townsend2010 commented May 13, 2019 •

edited

Loading