Desktop: Resolves #1752: Added capability to toggle visibility of the Menu Bar from the View menu #10324

LightTreasure · 2024-04-18T03:55:05Z

Add menu option and shortcut to toggle visibility of the menu bar

What this does:

This PR adds a new item to the View Menu called "Toggle Menu Bar", and proposes Ctrl+Shift+M as its default keyboard shortcut. Clicking on this item or using the shortcut toggles the visibility of the menu bar. Also, when the bar is hidden, pressing Alt will temporarily show the menu bar
EDIT: The menu item is not shown on Mac since macOS does not allow for hiding menus.

This feature has been requested via the following issues:

It has also been requested on the Joplin Forum:

How implemented:

Builds on @tinyoverflow's previously submitted pull request to address the same issue. This work incorporates @laurent22's feedback in that PR to add this capability as a View Menu item instead of it being present on the settings page.

Added a new entry to the 'View' Menu, called "Toggle menu bar", with a proposed keyboard shortcut Ctrl+Shift+M (defined in lib/services/KeymapService.ts), and a localization string added to tools/locales/joplin.pot and english translation tools/locales/en_US.po
Added a new, non-public setting called hideMenuBar, a boolean with the default value of false, applicable only to the Desktop App (defined in lib/models/Setting.ts)
Created a MainScreen Command called toggleMenuBar(), which contains the main implementation. This flips the value of the hideMenuBar setting and then correspondingly calls setAutoHideMenuBar() and setMenuBarVisibility() on the Electron Window.
Added a similar function called updateMenuBar() in app-desktop/app.ts - Called during initializatoin, this function calls setAutoHideMenuBar() and setMenuBarVisibility() based on the value of the hideMenuBar setting.
toggleMenuBar() and updateMenuBar() do not do anything on Mac.

Usage with screenshots:

The new menu entry:

When the setting is toggled:

Toggling again brings the menu back:

Issues:

Sometimes, after hiding the menu bar, the window is left with a white empty area, indicating that the contents aren't painted correctly. @tinyoverflow also mentioned this issue in their PR. I've tried debugging this, but haven't been able to fix it. My guess is that Electron isn't repainting the window properly after the menu bar is hidden.

github-actions · 2024-04-18T03:55:20Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

LightTreasure · 2024-04-18T03:56:40Z

I have read the CLA Document and I hereby sign the CLA

laurent22 · 2024-04-20T13:09:16Z

packages/tools/locales/en_US.po

+#: packages/app-desktop/gui/MainScreen/commands/toggleMenuBar.ts:9
+msgid "Toggle menu bar"
+msgstr ""
+


You don't need to add this here - that will be done automatically as long as you wrap the translatable strings in _(...) which you did

I've removed this in the latest commit

laurent22 · 2024-04-20T13:10:19Z

packages/app-desktop/gui/MainScreen/commands/toggleMenuBar.ts

+ // Now apply the setting
+ electronWindow.setAutoHideMenuBar(Setting.value('hideMenuBar'));
+ electronWindow.setMenuBarVisibility(!Setting.value('hideMenuBar'));


Please remove, since you're already doing this in the reducer middleware

I've removed this file and the references to this command in the latest commit

laurent22 · 2024-04-20T13:11:20Z

packages/lib/models/Setting.ts

+ hideMenuBar: {
+ value: false,
+ type: SettingItemType.Bool,
+ public: false,
+ appTypes: [AppType.Desktop],
+ },


Sorry would you mind naming it "showMenuBar" and setting it to true by default? This is the pattern we try to follow to avoid double-negations.

Agreed, I'll change it.

Changed in the latest commit.

laurent22 · 2024-04-20T13:16:55Z

packages/app-desktop/app.ts

+ public updateMenuBar() {
+ // macOS disallows the removal of the menu bar
+ if (shim.isMac()) return;
+
+ const window = bridge().window();
+ window.setAutoHideMenuBar(Setting.value('hideMenuBar'));
+ window.setMenuBarVisibility(!Setting.value('hideMenuBar'));
+ }


We have a MenuBar component to handle the menu. Would you mind moving the logic over there? If you need some help implementing this please let me know.

Essentially you'll need to add this to mapStateToProps:

showMenuBar: state.settings.showMenuBar,

Then check the value of props.showMenuBar from the component and hide/show it based on this.

The reason for doing it that way is to encapsulate all menu bar related logic in the same place, and to avoid the need to sync the settings and the state in the middleware.

Thanks for the review! Makes sense - I'll try implementing it this way and reply back here.

Moved the implementation to the MenuBar component in the latest commit. That simplifies things a lot!

…omponent, and changed the name of the setting to "showMenuBar"

laurent22

Nearly there but please bring back the command you had created

laurent22 · 2024-04-25T12:55:53Z

packages/app-desktop/gui/MenuBar.tsx

@@ -190,6 +191,15 @@ function menuItemSetEnabled(id: string, enabled: boolean) {
 menuItem.enabled = enabled;
 }

+function applyMenuBarVisibility(props: Props) {


const applyMenuBarVisibility = (props:Props => { (see coding style guide)

And actually only pass to the function that variables that you need - so just "showMenuBar", not the whole props

Whoops. Fixed.

laurent22 · 2024-04-25T12:58:54Z

packages/app-desktop/gui/MenuBar.tsx

+ shim.isMac() ? noItem : {
+ label: _('Toggle menu bar'),
+ accelerator: keymapService.getAccelerator('toggleMenuBar'),
+ click: () => {
+ Setting.setValue('showMenuBar', !Setting.value('showMenuBar'));
+ applyMenuBarVisibility(props);
+ },
+ },
+ // shim.isMac() ? noItem : menuItemDic.toggleMenuBar,


Hmm, but I'm not sure why you removed your command? That was the correct way to implement this, since it would be a command that can also be triggered from user scripts. It will also automatically register the keyboard shortcut.

Once the command is created, you can add it to menuCommandName.ts and then add it to the menu using the menuItemDic.xxxx pattern (see other commands in the menu)

Apologies - I clicked on "View Reviewed changes" in GitHub. For some reason, in that view, GitHub doesn't show which lines the comment applied to. I assumed you meant that I should remove the entire file.

Restored the command and used it for the implementation.

laurent22 · 2024-04-25T12:59:58Z

packages/tools/locales/joplin.pot

+#: packages/app-desktop/gui/MenuBar.tsx:774
+msgid "Toggle menu bar"
+msgstr ""
+


Please remove - this will be done automatically

Ah I see. Not very familiar with this style of localization. Removed it.

laurent22

That looks good now, thanks @LightTreasure. I'd just remove the isMac check in the command since it's redundant

laurent22 · 2024-04-27T10:38:40Z

packages/app-desktop/gui/MainScreen/commands/toggleMenuBar.ts

+ // Defensive code: macOS disallows hiding the menu bar, so ignore this Command
+ if (shim.isMac()) return;


At this stage it's not necessary to handle this, since you already handle it where it matters

Oh yup that makes sense. Removed in the newest commit.

…ince the core implementation has moved

LightTreasure · 2024-05-06T19:39:10Z

Hi @laurent22, please let me know if you have any further suggestions. Thanks so much!

laurent22 · 2024-05-07T09:56:58Z

That looks good now, thanks @LightTreasure!

Add View Menu item to toggle visibility of the Menu Bar

736ae78

github-actions bot added a commit that referenced this pull request Apr 18, 2024

@LightTreasure has signed the CLA in #10324

ed9b4fb

Hid the 'toggleMenuBar' entry on Mac

a16097e

laurent22 reviewed Apr 20, 2024

View reviewed changes

Moved implementation of menu bar visibility toggling to the MenuBar c…

0aa1da8

…omponent, and changed the name of the setting to "showMenuBar"

LightTreasure requested a review from laurent22 April 22, 2024 04:57

laurent22 reviewed Apr 25, 2024

View reviewed changes

Restored command for toggling menu bar, removed pot entry

bb2da50

LightTreasure requested a review from laurent22 April 26, 2024 22:08

laurent22 reviewed Apr 27, 2024

View reviewed changes

Removed isMac() check from the toggleMenuBar command implementation s…

17f2b7b

…ince the core implementation has moved

LightTreasure requested a review from laurent22 April 27, 2024 22:12

laurent22 merged commit 548ba7d into laurent22:dev May 7, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: Resolves #1752: Added capability to toggle visibility of the Menu Bar from the View menu #10324

Desktop: Resolves #1752: Added capability to toggle visibility of the Menu Bar from the View menu #10324

LightTreasure commented Apr 18, 2024 •

edited

github-actions bot commented Apr 18, 2024 •

edited

LightTreasure commented Apr 18, 2024

laurent22 Apr 20, 2024

LightTreasure Apr 22, 2024

laurent22 Apr 20, 2024

LightTreasure Apr 22, 2024

laurent22 Apr 20, 2024

LightTreasure Apr 20, 2024

LightTreasure Apr 22, 2024

laurent22 Apr 20, 2024

LightTreasure Apr 20, 2024

LightTreasure Apr 22, 2024

laurent22 left a comment

laurent22 Apr 25, 2024

laurent22 Apr 25, 2024

LightTreasure Apr 26, 2024

laurent22 Apr 25, 2024

LightTreasure Apr 26, 2024

LightTreasure Apr 26, 2024

laurent22 Apr 25, 2024

LightTreasure Apr 26, 2024

laurent22 left a comment

laurent22 Apr 27, 2024

LightTreasure Apr 27, 2024

LightTreasure commented May 6, 2024

laurent22 commented May 7, 2024

		// Defensive code: macOS disallows hiding the menu bar, so ignore this Command
		if (shim.isMac()) return;

Desktop: Resolves #1752: Added capability to toggle visibility of the Menu Bar from the View menu #10324

Desktop: Resolves #1752: Added capability to toggle visibility of the Menu Bar from the View menu #10324

Conversation

LightTreasure commented Apr 18, 2024 • edited

Add menu option and shortcut to toggle visibility of the menu bar

What this does:

How implemented:

Usage with screenshots:

Issues:

github-actions bot commented Apr 18, 2024 • edited

LightTreasure commented Apr 18, 2024

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

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

laurent22 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

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

laurent22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LightTreasure commented May 6, 2024

laurent22 commented May 7, 2024

LightTreasure commented Apr 18, 2024 •

edited

github-actions bot commented Apr 18, 2024 •

edited