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.

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

panelicon: add option to have both toggle and popup menu #349

Merged
merged 3 commits into from
Mar 6, 2023

Conversation

pesader
Copy link
Contributor

@pesader pesader commented Mar 6, 2023

As discussed in #328, I added a new panel button that includes the functionality of both previously available options (toggle and popup menu). I tested it using a touchpad and a mouse and it worked well on both cases.

I already ran the CI and it is failing at the step "Ensure translations are up to date", which I'm not sure how to fix 😅.

As usual, thanks for developing ddterm! 🚀

@amezin
Copy link
Member

amezin commented Mar 6, 2023

To fix translations, you need to run make msgmerge and add the changes to your commit (empty translation stubs for UI labels you've added)

@pesader
Copy link
Contributor Author

pesader commented Mar 6, 2023

I ran it, but I'm getting this error on the CI:

Error: po/fr.po:819: this message needs to be reviewed by the translator

I checked out the file location and found this:

#: ddterm/pref/glade/prefs-panel-icon.ui:57
msgid "Toggle button"
msgstr "Bouton de basculement"

#: ddterm/pref/glade/prefs-panel-icon.ui:58
#, fuzzy
msgid "Toggle button and popup menu"
msgstr "Bouton de basculement" # this line 819

Shouldn't that be empty?

@amezin
Copy link
Member

amezin commented Mar 6, 2023

Yes, it should be empty.

Likely I should rethink how translations are maintained/processed.

For now you can remove #, fuzzy line, and the "automatically guessed" translation

@pesader
Copy link
Contributor Author

pesader commented Mar 6, 2023

From reading the docs, it looks like it is possible to disable fuzzy matching with the flag --no-fuzzy-matching. Maybe that would solve it :)

@amezin amezin merged commit 53f9191 into ddterm:master Mar 6, 2023
@amezin
Copy link
Member

amezin commented Mar 6, 2023

Thank you and sorry for the inconvenience with translations

@amezin
Copy link
Member

amezin commented Mar 7, 2023

Could you refactor PanelIconToggleAndMenu to inherit from PanelIconPopupMenu to get rid of duplicate code?

@pesader
Copy link
Contributor Author

pesader commented Mar 7, 2023

I thought of doing it like that at first, but decided not to because:

  1. There's also a lot of code from PanelIconToggleButton, but I don't feel like inheriting just from PanelIconPopupMenu makes that clear.

  2. We discussed merging both PanelIconPopupMenu and PanelIconToggleButton later on, as a separate change, and inhenritance would make that transition more difficult.

I can still do the refactor if you prefer, but this is my reasoning.

@amezin
Copy link
Member

amezin commented Mar 7, 2023

  1. There's also a lot of code from PanelIconToggleButton, but I don't feel like inheriting just from PanelIconPopupMenu makes that clear.

The menu has higher chance of being changed. Right now, if you want to add a menu item, you'll have to do it in two places. And there's not much that can be changed in a simple toggle button, so duplication here is a smaller problem.

  1. We discussed merging both PanelIconPopupMenu and PanelIconToggleButton later on, as a separate change, and inhenritance would make that transition more difficult.

Not merging, just removing PanelIconToggleButton and configuration options. And PanelIconPopupMenu can remain as a base class, or can be merged back into PanelIconToggleAndMenu.

@pesader
Copy link
Contributor Author

pesader commented Mar 7, 2023

The menu has higher chance of being changed. Right now, if you want to add a menu item, you'll have to do it in two places.

Yeah makes sense. Thanks for explaining!

pesader added a commit to pesader/gnome-shell-extension-ddterm that referenced this pull request Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants