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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
May 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
736ae78
Add View Menu item to toggle visibility of the Menu Bar
LightTreasure Apr 17, 2024
a16097e
Hid the 'toggleMenuBar' entry on Mac
LightTreasure Apr 18, 2024
0aa1da8
Moved implementation of menu bar visibility toggling to the MenuBar c…
LightTreasure Apr 22, 2024
bb2da50
Restored command for toggling menu bar, removed pot entry
LightTreasure Apr 26, 2024
17f2b7b
Removed isMac() check from the toggleMenuBar command implementation s…
LightTreasure Apr 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.test.js
packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.js
packages/app-desktop/gui/MainScreen/commands/toggleEditors.js
packages/app-desktop/gui/MainScreen/commands/toggleLayoutMoveMode.js
packages/app-desktop/gui/MainScreen/commands/toggleMenuBar.js
packages/app-desktop/gui/MainScreen/commands/toggleNoteList.js
packages/app-desktop/gui/MainScreen/commands/toggleNoteType.js
packages/app-desktop/gui/MainScreen/commands/toggleNotesSortOrderField.js
Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.test.js
packages/app-desktop/gui/MainScreen/commands/showSpellCheckerMenu.js
packages/app-desktop/gui/MainScreen/commands/toggleEditors.js
packages/app-desktop/gui/MainScreen/commands/toggleLayoutMoveMode.js
packages/app-desktop/gui/MainScreen/commands/toggleMenuBar.js
packages/app-desktop/gui/MainScreen/commands/toggleNoteList.js
packages/app-desktop/gui/MainScreen/commands/toggleNoteType.js
packages/app-desktop/gui/MainScreen/commands/toggleNotesSortOrderField.js
Expand Down
13 changes: 13 additions & 0 deletions packages/app-desktop/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ class Application extends BaseApplication {
webFrame.setZoomFactor(Setting.value('windowContentZoomFactor') / 100);
}

if (!shim.isMac() && (action.type === 'SETTING_UPDATE_ONE' && action.key === 'hideMenuBar' || action.type === 'SETTING_UPDATE_ALL')) {
this.updateMenuBar();
}

if (action.type === 'SETTING_UPDATE_ONE' && action.key === 'linking.extraAllowedExtensions' || action.type === 'SETTING_UPDATE_ALL') {
bridge().extraAllowedOpenExtensions = Setting.value('linking.extraAllowedExtensions');
}
Expand Down Expand Up @@ -201,6 +205,15 @@ class Application extends BaseApplication {
}
}

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'));
}
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


public updateEditorFont() {
const fontFamilies = [];
if (Setting.value('style.editor.fontFamily')) fontFamilies.push(`"${Setting.value('style.editor.fontFamily')}"`);
Expand Down
2 changes: 2 additions & 0 deletions packages/app-desktop/gui/MainScreen/commands/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import * as showShareNoteDialog from './showShareNoteDialog';
import * as showSpellCheckerMenu from './showSpellCheckerMenu';
import * as toggleEditors from './toggleEditors';
import * as toggleLayoutMoveMode from './toggleLayoutMoveMode';
import * as toggleMenuBar from './toggleMenuBar';
import * as toggleNoteList from './toggleNoteList';
import * as toggleNoteType from './toggleNoteType';
import * as toggleNotesSortOrderField from './toggleNotesSortOrderField';
Expand Down Expand Up @@ -88,6 +89,7 @@ const index: any[] = [
showSpellCheckerMenu,
toggleEditors,
toggleLayoutMoveMode,
toggleMenuBar,
toggleNoteList,
toggleNoteType,
toggleNotesSortOrderField,
Expand Down
26 changes: 26 additions & 0 deletions packages/app-desktop/gui/MainScreen/commands/toggleMenuBar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { CommandDeclaration, CommandRuntime } from '@joplin/lib/services/CommandService';
import bridge from '../../../services/bridge';
import shim from '@joplin/lib/shim';
import Setting from '@joplin/lib/models/Setting';
import { _ } from '@joplin/lib/locale';

export const declaration: CommandDeclaration = {
name: 'toggleMenuBar',
label: () => _('Toggle menu bar'),
};

export const runtime = (): CommandRuntime => {
return {
execute: async () => {
// Defensive code: macOS disallows hiding the menu bar, so ignore this Command
if (shim.isMac()) return;
Copy link
Owner

Choose a reason for hiding this comment

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

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

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 yup that makes sense. Removed in the newest commit.


const electronWindow = bridge().window();
Setting.toggle('hideMenuBar');

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

Choose a reason for hiding this comment

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

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

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've removed this file and the references to this command in the latest commit

},
};
};
1 change: 1 addition & 0 deletions packages/app-desktop/gui/MenuBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,7 @@ function useMenu(props: Props) {
menuItemDic.resetLayout,
separator(),
menuItemDic.toggleSideBar,
shim.isMac() ? noItem : menuItemDic.toggleMenuBar,
menuItemDic.toggleNoteList,
menuItemDic.toggleVisiblePanes,
{
Expand Down
1 change: 1 addition & 0 deletions packages/app-desktop/gui/menuCommandNames.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export default function() {
'toggleExternalEditing',
'toggleLayoutMoveMode',
'resetLayout',
'toggleMenuBar',
'toggleNoteList',
'toggleNotesSortOrderField',
'toggleNotesSortOrderReverse',
Expand Down
7 changes: 7 additions & 0 deletions packages/lib/models/Setting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1283,6 +1283,13 @@ class Setting extends BaseModel {
isGlobal: true,
},

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in the latest commit.


startMinimized: { value: false, type: SettingItemType.Bool, storage: SettingStorage.File, isGlobal: true, section: 'application', public: true, appTypes: [AppType.Desktop], label: () => _('Start application minimised in the tray icon') },

collapsedFolderIds: { value: [], type: SettingItemType.Array, public: false },
Expand Down
1 change: 1 addition & 0 deletions packages/lib/services/KeymapService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ const defaultKeymapItems = {
{ accelerator: 'Ctrl+Shift+N', command: 'focusElementNoteTitle' },
{ accelerator: 'Ctrl+Shift+B', command: 'focusElementNoteBody' },
{ accelerator: 'F10', command: 'toggleSideBar' },
{ accelerator: 'Ctrl+Shift+M', command: 'toggleMenuBar' },
{ accelerator: 'F11', command: 'toggleNoteList' },
{ accelerator: 'Ctrl+L', command: 'toggleVisiblePanes' },
{ accelerator: 'Ctrl+0', command: 'zoomActualSize' },
Expand Down
4 changes: 4 additions & 0 deletions packages/tools/locales/en_US.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4938,6 +4938,10 @@ msgstr ""
msgid "Toggle external editing"
msgstr ""

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

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've removed this in the latest commit

#: packages/lib/models/Setting.ts:2714
msgid "Toggle note history, keep notes for"
msgstr ""
Expand Down
4 changes: 4 additions & 0 deletions packages/tools/locales/joplin.pot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4651,6 +4651,10 @@ msgstr ""
msgid "Toggle external editing"
msgstr ""

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

#: packages/lib/models/Setting.ts:2714
msgid "Toggle note history, keep notes for"
msgstr ""
Expand Down