-
Notifications
You must be signed in to change notification settings - Fork 20
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
Minimized to Tray #167
base: main
Are you sure you want to change the base?
Minimized to Tray #167
Conversation
lemonxah
commented
Jan 16, 2023
- minimize to tray preference
- added tray icon
- minimze to tray
- startup minimized
I would vote for a such feature :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lemonxah, many thanks for the PR.
Sorry for not reacting to it earlier. jack_mixer
isn't really properly maintained at the moment. I only do the things that need to be done if nobody else is doing them.
Anyway, I reviewed your PR now but I'm afraid many changes are necessary before it can be merged. I made many comments with suggested changes, but I'm almost sure there are even more things, I just ran out of time.
One fundamental issue is that the feature doesn't really do what the title of the PR says. It intercepts quitting of the application and allows hiding/showing it from the tray menu. Minimizing the window is unaffected.
IMHO, minimizing should be intercepted and the window hidden, not minimized, if the tray item is available and the "Minimize to tray" option is set.
Quitting the application via the menu or Ctrl-Q should not minimize it to the tray, or there should be an additional option for it.
Closing the window via the window decoration close button should probably be the same as minimizing. But I'm open for other opinions on that.
I can understand if you don't want to do these extensive changes after such a long time. If you don't update the PR within the next few weeks, I'll probably do it.
exit(1) | ||
|
||
try: | ||
from gi.repository import AppIndicator3 as appindicator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This triggers the following warning. Please insert the appropriate gi.require_version
call.
PyGIWarning: AppIndicator3 was imported without specifying a version first. Use gi.require_version('AppIndicator3', '0.1') before import to ensure that the right version gets loaded.
self.tray_minimized_checkbutton = Gtk.CheckButton(_("Minimize to tray")) | ||
self.tray_minimized_checkbutton.set_tooltip_text( | ||
_("Minimize the application to the system tray when the window is closed") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the translation message catalog for these new translatable strings.
[Unit] | ||
Description=Jack Mixer Service | ||
Requires=pipewire.socket | ||
After=graphical.target pwg.service | ||
|
||
[Service] | ||
Type=simple | ||
ExecStart=/usr/local/bin/jack_mixer --minimized --config /home/lemonxah/.config/jack_mixer/lemonxah.xml | ||
ExecStop=/bin/kill -9 $MAINPID | ||
Restart=always | ||
|
||
[Install] | ||
WantedBy=default.target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a different feature. Please make a separate PR for it and remove it from this one.
@@ -103,6 +104,9 @@ def __init__(self, client_name=__program__): | |||
self.last_xml_serialization = None | |||
self.cached_xml_serialization = None | |||
|
|||
log.info("Starting %s %s", __program__, __version__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the log level to DEBUG.
@@ -719,7 +723,14 @@ def on_save_as_cb(self, *args): | |||
dlg.destroy() | |||
|
|||
def on_quit_cb(self, *args, on_delete=False): | |||
if not self.nsm_client and self.gui_factory.get_confirm_quit(): | |||
log.info("on_quit_cb") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this log call.
self.separator = Gtk.SeparatorMenuItem() | ||
self.menu.append(self.separator) | ||
|
||
self.exittray = Gtk.MenuItem(label = 'Quit') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string should be translatable.
|
||
def create_menu(self): | ||
self.menu = Gtk.Menu() | ||
self.menu.set_title('Jack Mixer') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title should not be hard-coded here. I suggest you make it an init keyword arg and pass it from app.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example:
def __init__(self, app, name, icon=None):
self.available = bool(appindicator)
if appindicator is None:
log.warning('AppIndicator3 not found, indicator will not be available')
return
self.app = app
self.name = name
self.indicator = appindicator.Indicator.new(
name,
icon or name,
appindicator.IndicatorCategory.APPLICATION_STATUS)
self.indicator.set_status(appindicator.IndicatorStatus.ACTIVE)
self.indicator.set_menu(self.create_menu())
def create_menu(self):
self.menu = Gtk.Menu()
self.menu.set_title(self.name)
....
"-m", | ||
"--minimized", | ||
action="store_true", | ||
default=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default for store_true
args is always False
. Please remove this line.
from gi.repository import Gtk | ||
from os import environ, path | ||
|
||
prefix = environ.get('MESON_INSTALL_PREFIX', '/usr') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work. MESON_INSTALL_PREFIX
is not set at run-time. Just use an icon name withoutt a path. It is the responsibility of the packaging to install the icon in a path where it will be found.
if not self.app.nsm_client and self.app.gui_factory.get_confirm_quit(): | ||
dlg = Gtk.MessageDialog( | ||
parent=self.app.window, | ||
message_type=Gtk.MessageType.QUESTION, | ||
buttons=Gtk.ButtonsType.NONE, | ||
) | ||
dlg.set_markup(_("<b>Quit application?</b>")) | ||
dlg.format_secondary_markup( | ||
_( | ||
"All jack_mixer ports will be closed and connections lost," | ||
"\nstopping all sound going through jack_mixer.\n\n" | ||
"Are you sure?" | ||
) | ||
) | ||
dlg.add_buttons( | ||
Gtk.STOCK_CANCEL, Gtk.ResponseType.CANCEL, Gtk.STOCK_QUIT, Gtk.ResponseType.OK | ||
) | ||
response = dlg.run() | ||
dlg.destroy() | ||
if response != Gtk.ResponseType.OK: | ||
return on_delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't duplicate code from app.py
here. Refactor the code after the elif
from on_quit_cb
into it's own method and call that method from here (via self.app
).