Skip to content

Added feature to export/import menus! #1230

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
Open
yar2000T wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Added feature to export/import menus! #1230

yar2000T wants to merge 9 commits into from

Conversation

@yar2000T
Copy link
Contributor

@yar2000T yar2000T commented Dec 24, 2025

No description provided.

@yar2000T yar2000T mentioned this pull request Dec 24, 2025
Open
@Schneegans
Copy link
Contributor

Schneegans commented Dec 31, 2025

This looks quite good now! I think there are two things which we could improve:

  1. I think you could remove the showSaveDialog and showErrorDialog from the settings API. Simply put all this logic in the settings-window.export-menu and settings-window.import-menu handlers respectively. This would safe us some round trips between the processes and would make the code easier to read, I think.
  2. The error message if an invalid JSON is imported is really hard to read. Can this be improved somehow?

@yar2000T
Copy link
Contributor Author

yar2000T commented Jan 1, 2026

I dont know what we can do abot redability of errors, I changed layout of error window and applied your 1 suggestion

@yar2000T
Copy link
Contributor Author

yar2000T commented Jan 9, 2026

Any updates on this?

Schneegans
Schneegans requested changes Jan 10, 2026
View reviewed changes
Copy link
Contributor

@Schneegans Schneegans left a comment

Choose a reason for hiding this comment

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

I think it works quite well now! I just added some more suggestions below 😄

src/main/app.ts Outdated
if (menuIndex < 0 || menuIndex >= settings.menus.length) {
await dialog.showMessageBox(this.settingsWindow, {
type: 'error',
title: i18next.t('settings.export-menu-error-title', 'Failed to export menu'),
Copy link
Contributor

@Schneegans Schneegans Jan 10, 2026

Choose a reason for hiding this comment

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

I think there is no need to show a translatable message box here. This should only happen on a coding error, not on user error. So a simple console.error(...) should be sufficient.

Copy link
Contributor Author

@yar2000T yar2000T Jan 10, 2026

Choose a reason for hiding this comment

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

Updated!

src/main/app.ts Outdated
Comment on lines 875 to 877
// Convert the exported root into a full MENU object so defaults (like
// centered/anchored/hoverMode and shortcut fields) are applied.
const validatedMenu = MENU_SCHEMA_V1.parse({ root: exported.menu }, { reportInput: true });
Copy link
Contributor

@Schneegans Schneegans Jan 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I think this second parse should not be necessary. The first parsing in the lines above this already fully parses the menu. You can just access it with exported.menu.

Copy link
Contributor Author

@yar2000T yar2000T Jan 10, 2026

Choose a reason for hiding this comment

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

EXPORTED_MENU_SCHEMA_V1 only validates a MenuItem, and the second parse is required to construct a full Menu object and apply defaults.

Copy link
Contributor

@Schneegans Schneegans Jan 10, 2026

Choose a reason for hiding this comment

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

Ah, I see. You are right!

src/main/app.ts Outdated
await dialog.showMessageBox(this.settingsWindow, {
type: 'error',
title: i18next.t('settings.import-menu-error-title', 'Failed to import menu'),
message: 'The selected file could not be imported. It does not contain a valid Kando exported menu.',
Copy link
Contributor

@Schneegans Schneegans Jan 10, 2026

Choose a reason for hiding this comment

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

This should be translatable.

Copy link
Contributor Author

@yar2000T yar2000T Jan 10, 2026

Choose a reason for hiding this comment

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

Added to translations

src/main/app.ts Outdated

await dialog.showMessageBox(this.settingsWindow, {
type: 'error',
title: i18next.t('settings.import-menu-error-title', 'Failed to import menu'),
Copy link
Contributor

@Schneegans Schneegans Jan 10, 2026

Choose a reason for hiding this comment

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

We usually do not add fallbacks in the code.

Suggested change
title: i18next.t('settings.import-menu-error-title', 'Failed to import menu'),
title: i18next.t('settings.import-menu-error-title'),

Copy link
Contributor Author

@yar2000T yar2000T Jan 10, 2026

Choose a reason for hiding this comment

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

Fixed!

src/settings-renderer/settings-window-api.ts Outdated
// SPDX-License-Identifier: MIT

import { ipcRenderer, OpenDialogOptions, webFrame } from 'electron';
import { ipcRenderer, OpenDialogOptions, SaveDialogOptions, webFrame } from 'electron';
Copy link
Contributor

@Schneegans Schneegans Jan 10, 2026

Choose a reason for hiding this comment

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

I think this change could be undone, right?

Copy link
Contributor Author

@yar2000T yar2000T Jan 10, 2026

Choose a reason for hiding this comment

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

Good point!

@yar2000T
Copy link
Contributor Author

yar2000T commented Jan 10, 2026

Okay, I’ll fix your suggestions later today.
Do you know if this will be included in 2.2.0, or will it come with 2.3.0?

yar2000T and others added 4 commits January 10, 2026 10:32
@yar2000T @Schneegans
Update src/main/app.ts
56a6d96 
Co-authored-by: Simon Schneegans <code@simonschneegans.de>
@yar2000T
Merge pull request #1 from kando-menu/main
27029eb 
Merge `main` into `feature/export-import-menus`
@yar2000T
Applied suggestions
5d199d2
@yar2000T
Applied lint
3a39116
@Schneegans
Copy link
Contributor

Schneegans commented Jan 10, 2026

More likely 2.3.0, I guess. Then we can properly advertise and test it. I am also experimenting with another new 2.3.0 feature which would fit quite well:

Screencast.From.2026-01-10.20-28-22.mp4

I do not like too much how there are lots of buttons now in the floating button bar at the bottom. Maybe we can use Kando more often in... Kando 😉

@yar2000T
Copy link
Contributor Author

yar2000T commented Jan 11, 2026 •
edited
Loading

Pretty cool idea! It will entertain users to use Kando.

@yar2000T
Copy link
Contributor Author

yar2000T commented Jan 11, 2026

Any else suggestions on code?

Schneegans
Schneegans approved these changes Jan 11, 2026
View reviewed changes
Copy link
Contributor

@Schneegans Schneegans left a comment

Choose a reason for hiding this comment

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

I think code-wise, this looks great! But as mentioned before, I will wait with the merge until 2.2.0 is released. Thank you so much!

@yar2000T
Copy link
Contributor Author

yar2000T commented Jan 12, 2026

Okay, perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Schneegans Schneegans Schneegans approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yar2000T @Schneegans