Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

refactor: Update Fether menu for taskbar mode on non-macOS (add 'Quit', remove 'Close Window', remove 'Minimize') #440

Merged

Conversation

ltfschoen
Copy link
Contributor

@ltfschoen ltfschoen commented Feb 21, 2019

  • Refactor Fether menu code

  • Add context menu that appears when you click on the system tray icon on Linux and Windows, allowing the Fether window to be "frameless" and we don't need to use ALT key to toggle it anymore. Not required for macOS since we can keep the Fether window "frameless" and use the Fether menu that appears in the taskbar

  • Add File > Quit to Fether menu when not using macOS, since macOS has a different File menu items

  • Remove the Fether menu tab "Window" entirely and associated menu items when running in taskbar mode, since we are only allowing user to either blur the window, hide and re-open it by clicking the taskbar icon, or quit

  • Remove adjustment of window height to in 'app-resize' since we are now using "frameless" Fether window on all OS

@ltfschoen
Copy link
Contributor Author

Note that subsequent to this PR we need to make the Fether context menu appear with a "frameless" Fether window on all OS

@Tbaut
Copy link
Collaborator

Tbaut commented Feb 21, 2019

  • Add File > Quit to Fether menu when not using macOS

I don't quite understand. Since it's frameless, there is no menu on Linux (as it doesn't work, and that's fine because there's no need for it).
But then there is no "File>Quit" as well. Hence #438

@ltfschoen
Copy link
Contributor Author

  • Add File > Quit to Fether menu when not using macOS

I don't quite understand. Since it's frameless, there is no menu on Linux (as it doesn't work, and that's fine because there's no need for it).
But then there is no "File>Quit" as well. Hence #438

It's possible to have the context menu appear as a popup when you right-click on a "frameless" Fether window, and even have the context menu appear as a popup when you right-click on the system tray icon, as shown in this screenshot
screen shot 2019-02-21 at 7 06 18 pm

I think @axelchalon's comment #433 (comment) was referring to the fact that there wasn't a "Quit" menu item at all for Windows and Linux in the Fether menu, we only had one on macOS.

I've changed this PR back to being in-progress.

Previously when I used tray.setContextMenu in this commit to show the context menu popup on right-click, I was able to get the context menu popup to appear, but when I clicked the menu items it didn't respond 8ce7add

Whereas today I was able to show the context menu popup on right-click, and the items worked when using remote approach in the front-end instead, as mentioned here https://electronjs.org/docs/api/menu#render-process

I think we need the Fether context menu on Windows and Linux, and I'm going to try and get the right-click popup context menu to work.

@ltfschoen ltfschoen force-pushed the luke-refactor-menu-and-no-close-minimise-window-for-taskbar-app branch from 14e5210 to 63a73cc Compare February 21, 2019 22:16
@ltfschoen
Copy link
Contributor Author

ltfschoen commented Feb 21, 2019

@Tbaut I've pushed commits so that on Linux and Windows if you click the system tray icon it shows a context menu. It all works on both Ubuntu Gnome OS and Linux Mint OS. There's just an issue on Windows OS where clicking the menu items in the "Edit" and "Window" sections of the menu doesn't work, and I do not understand why....

Linux Mint - clicking system tray icon toggles context menu that works
screen shot 2019-02-21 at 11 24 32 pm

Ubuntu Gnome - clicking system tray icon toggles context menu that works
screen shot 2019-02-22 at 12 38 32 am

Windows 10 - clicking dock icon toggles Fether window that works, and balloon shows usage instructions. however whilst clicking system tray icon toggles context menu, the "Edit" and "Window" menu items don't work when you click them

screen shot 2019-02-21 at 11 59 10 pm

screen shot 2019-02-22 at 12 13 09 am

@ltfschoen
Copy link
Contributor Author

ltfschoen commented Feb 22, 2019

Update on Windows issue where context menu 'Edit' and 'View' menu tab menu items not working:

If I configure the menu using webContents I've been able to get all the menu items in the 'View' tab working on Windows 10. I've also used the same approach to get all the menu items in the 'Edit' tab to work on Windows 10, which also requires me to manually import the instance of Fether app (i.e. fetherAppInstance and then run fetherAppInstance.win.focus() when tray.on('click' is called, which is one of the event listeners that fortunately works on Windows!

Note that in the 'Window' menu tab, on Windows OS, I've only included "Reload" and "Toggle Developer Tools", since they are the only ones that appear to be available in webContents. I also think "Force Reload", the zooming menu items, and the menu item that toggles full screen aren't helpful to users anyway. What do you think? Should we remove those menu items from Linux and macOS too to KISS?

Note that I've added to the "Edit" menu tab the "Undo" and "Redo" menu items too.

Note that the following resources were most helpful:

@Tbaut
Copy link
Collaborator

Tbaut commented Feb 22, 2019

Thanks a lot for the thorough testing.
TBH I really think it's overkill to have so many tray menus. All I need as a user is "Quit Fether" and it's now "hidden" behind "Electron" which as a random user have no idea what this means 🤓. Also I do not expect to have edit, copy, paste etc.. in such a menu. I've never seen this on other applications.

What Riot does and I believe we should do as well (keep it simple):

  • Tray icon:

    • Show/Hide Riot
    • Quit
  • Left click on a selection (I don't think we really need that for now, shortcut are enough):

    • Cut
    • Copy
    • Paste
    • Paste without style (no need for us)
    • Select all (no need for us)

Let's discuss all this beforehand in an issue so that we only implement what makes a consensus.

@ltfschoen
Copy link
Contributor Author

ltfschoen commented Feb 22, 2019

Update: I've added About and Quit (not Exit as shown) to the bottom of the context menu on Windows and Linux, and removed File and Help menus

screen shot 2019-02-22 at 6 57 22 pm

@ltfschoen ltfschoen force-pushed the luke-refactor-menu-and-no-close-minimise-window-for-taskbar-app branch from 3c29239 to 42b4065 Compare February 23, 2019 12:26
… so unable to toggle window show/hide as well
@ltfschoen
Copy link
Contributor Author

ltfschoen commented Feb 23, 2019

@Tbaut I've pushed a commit that displays the context menu when you "right-click" in the Fether window that contains the Edit, View, and Help submenus, and Quit. I know we discussed only having the Edit menu items appear when you "right-click" in the Fether window, but I think it's better also having the the View and Help submenus and Quit item there too (rather than adding confusion by making the user have to click the "tray" icon instead to access those other menu items). So on that basis I've also removed the feature that also displayed the same context menu when you clicked the "tray" icon, so now clicking the tray icon just shows/hides the Fether window as it did before.

Please try it out and let me know what you think?

Note that one of the main reasons why I've proposed to not rely on showing the menu when the user right-clicks on the "tray" icon, is because on Linux (i.e. Ubuntu Gnome) right-clicking on the tray icon is not supported (API docs show only support for macOS and Windows https://electronjs.org/docs/api/tray#event-right-click-macos-windows). See #422

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

It's great for the left click menu in Fether iindow.

Due to not being able to avoid this and the fact that right-click on the tray icon doesn't work on Linux either, I've proposed that for all OSes

One of the most important notion in UX is to not introduce a behavior that users are not used to, unless you have a really good excuse (keeping things consistent across OSes doesn't matter to users).
We should:

  • Keep the behavior of hide/appear on left click for any OS that supports is (all except Linux)
  • Let a "Quit" menu entry on the tray menu no matter what.

Summary:

Keep the menu as it is in the window, it's great
Linux:

  • Left and right click on the tray icon shows:
    • Show/Hide Fether
    • Quit

Mac and Windows:

  • Left click on the tray icon directly toggles Fether
  • Right click on the tray show:
    • Show/Hide Fether
    • Quit

function showTrayBalloon (fetherApp) {
const { tray } = fetherApp;

pino.info('Showing Tray Balloon');

tray.displayBalloon({
content: `Press ALT in the Fether window to toggle the menu`,
content:
'Click to toggle Fether window. Right-click Fether window to toggle Fether menu',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'Click to toggle Fether window. Right-click Fether window to toggle Fether menu',
'Show/Hide Fether',

This is not true for Linux. Since it's a menu item we should keep things short and simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should we communicate to users that they can view the full menu by right-clicking on the Fether window?

@ltfschoen
Copy link
Contributor Author

Due to not being able to avoid this and the fact that right-click on the tray icon doesn't work on Linux either, I've proposed that for all OSes

I've since discovered that we can show the tray menu with "Show/Hide Fether" and "Quit" items by running tray.popUpContextMenu(); after we set the context menu that we want to use i.e. tray.setContextMenu(fetherApp.contextTrayMenu.getMenu()); in the tray.on('right-click' listener

Linux:

  • Left and right click on the tray icon shows:

    • Show/Hide Fether
    • Quit

This works!

Mac and Windows:

  • Left click on the tray icon directly toggles Fether

  • Right click on the tray show:

    • Show/Hide Fether
    • Quit

On macOS, when we configure the tray icon to display the context menu, it only runs the code in the 'right-click' listener when you either left-click or right-click the tray icon (i.e. 'click' and 'double-click' do not work). I think it's related to this issue.
But users can just select "Show/Hide Fether" and it'll show/hide the Fether window
So it ends up being the same functionality as on macOS.

On Windows, it all works the way we want it to (i.e. the left-click the tray icon toggles Fether directory and right-click shows the context menu with "Show/Hide Fether" and "Quit" items)

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

🎉 working as expected.

Last comment inline :)

Requesting @axelchalon code review as well :)

template.push({
role: 'help',
submenu: [
{ role: 'about' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clicking on "about" doesn't do anything. I'd suggest to keep only one of "about" or "Learn More" and point it to https://github.com/paritytech/fether instead (as Parity.io does).

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 left both because I thought we should give the user the ability to view their current Fether version, and also finding out more about Parity and Fether:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see what you mean, it doesn't work because the 'about' role is only supported on macOS according to the API docs https://electronjs.org/docs/api/menu-item#roles.
I'll push an update so it only appears on macOS

@Tbaut Tbaut merged commit 6ae3c1f into master Feb 27, 2019
@Tbaut Tbaut deleted the luke-refactor-menu-and-no-close-minimise-window-for-taskbar-app branch May 7, 2019 19:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants