Created attachment 301710 [details] [review]
StartUpTweak: Use g_application_mark_busy()

It can take a few seconds to create the AppChooser dialog so mark the
application as busy to provide some visual feedback when the ‘+’ button
is clicked.

Created attachment 301711 [details] [review]
AppChooser: Include matching running apps when filtering

Running apps have two labels so match against both of them rather than
failing if the first doesn't match. This ensures that any apps that
match and are running get included in the filtered list.

Created attachment 301712 [details] [review]
AppChooser: Don't show existing start-up apps

Pass a list of existing start-up applications to the AppChooser dialog so
it can avoid showing them.

Created attachment 301713 [details] [review]
AppChooser: Desensitize ‘Add’ button when no app is selected

The ‘Add Application’ button shouldn't be sensitive if there is no
application currently selected. This is complicated be the fact that
when Gtk.Listbox filters the list it does not clear the selection if the
selected row is filtered out so we need to watch for the selected row
being umapped.

Created attachment 301714 [details] [review]
AppChooser: Add a default response

Adding a default response improves the keyboard navigation as the user
can select an application by pressing ‘Enter’. It also changes the style
of the ‘Add Application’ button.

Created attachment 301715 [details] [review]
AppChooser: Select app when row is activated

This allows the user to select a startup application by double clicking
on it rather than having to select it and then click on the ‘Add
Application’ button.

Created attachment 301716 [details] [review]
AppChooser: Rationalize ‘Escape’ handling

Pressing ‘Escape’ when the search-bar is visible should close it, if the
search-bar is not visible it should close the dialog.

Created attachment 301717 [details] [review]
AppChooser: Respect setting of gtk-dialogs-use-header

Use a header-bar for the start-up application chooser dialog if
‘gtk-dialogs-use-header’ is True. Rename the ‘Add Application’ button to
‘Add’ and add a mnemonic for it as well as changing the response of the
‘Close’ button so that it is placed on the opposite side of the
header-bar to the ‘Add’ button.

Review of attachment 301710 [details] [review]:

ok

Review of attachment 301711 [details] [review]:

fine

Review of attachment 301712 [details] [review]:

::: gtweak/tweaks/tweak_group_startup.py
@@ +55,3 @@
         apps = Gio.app_info_get_all()
         for a in apps:
+            for b in startup_apps:

ugh, if we have a large number of apps here this might take a while

@@ +252,3 @@
     def _on_add_clicked(self, btn):
         Gio.Application.get_default().mark_busy()
+        startup_apps = []

can we make this a set instead? would avoid the issue above

Review of attachment 301715 [details] [review]:

++

Review of attachment 301713 [details] [review]:

Don't like the complexity here. Isn't it enough to set the ListBox selection mode to GTK_SELECTION_BROWSE ?

::: gtweak/tweaks/tweak_group_startup.py
@@ +56,3 @@
         self.entry.connect("search-changed", lambda e: lb.invalidate_filter())
 
+        lb.connect("row-activated", lambda b, r: self.response(Gtk.ResponseType.OK))

this is duplicated on a patch further ahead, should be removed here

Review of attachment 301714 [details] [review]:

++

Review of attachment 301716 [details] [review]:

I agree that this feels better

Review of attachment 301717 [details] [review]:

anyway, fine

::: gtweak/tweaks/tweak_group_startup.py
@@ +34,3 @@
     def __init__(self, main_window, running_exes, startup_apps):
+        uhb = Gtk.Settings.get_default().props.gtk_dialogs_use_header
+        Gtk.Dialog.__init__(self, title=_("Applications"), use_header_bar=uhb)

GtkDialog doesn't do this by itself?

(In reply to Rui Matos from comment #16)
> Review of attachment 301717 [details] [review] [review]:
> 
> anyway, fine
> 
> ::: gtweak/tweaks/tweak_group_startup.py
> @@ +34,3 @@
>      def __init__(self, main_window, running_exes, startup_apps):
> +        uhb = Gtk.Settings.get_default().props.gtk_dialogs_use_header
> +        Gtk.Dialog.__init__(self, title=_("Applications"),
> use_header_bar=uhb)
> 
> GtkDialog doesn't do this by itself?

Unfortunately it doesn't. Internally it uses gtk_dialog_set_use_header_bar_from_setting() for the derived dialogs (colorchooser etc) provided by Gtk but it does not export it.

Created attachment 302431 [details] [review]
AppChooser: Don't show existing start-up apps

I've updated this to use a set as you suggested. This probably causes
conflicts with some later patches so I'll push a branch
wip/pwood/app-chooser with the current versions rather than obsoleting
the patches you've already approved.

Pass a set containing the existing start-up applications to the
AppChooser dialog so it can avoid showing them.

Created attachment 302433 [details] [review]
AppChooser: Desensitize ‘Add’ button when no app is selected

In reply to comment 13
> Review of attachment 301713 [details] [review] [review]:
>
> Don't like the complexity here. Isn't it enough to set the ListBox
> selection mode to GTK_SELECTION_BROWSE ?

No, for some reason Gtk refuses to deselect a row when it's hidden by
the filter even though it looks to the user like there are no rows
selected (I wondered if it was due to the semantics of
GTK_SELECTION_BROWSE preventing the selection from being cleared, but
it is the same with GTK_SELECTION_SINGLE). I don't like it (I thought
this would be a quick change but it took quite a while to work out
what was going on) but this seems to be the only way to get it to
work. I've added a couple of comments to the code about this. This
patch now increases the required Gtk version as it needs 3.14 for
GtkListBox::row-selected.

>::: gtweak/tweaks/tweak_group_startup.py
>@@ +56,3 @@
>         self.entry.connect("search-changed", lambda e: lb.invalidate_filter())
>
>+        lb.connect("row-activated", lambda b, r: self.response(Gtk.ResponseType.OK))
>
>this is duplicated on a patch further ahead, should be removed here

Well spotted, it was from a bad rebase, I've fixed it.


The ‘Add Application’ button shouldn't be sensitive if there is no
application currently selected. This is complicated be the fact that
when Gtk.Listbox filters the list it does not clear the selection if the
selected row is filtered out so we need to watch for the selected row
being umapped.

Review of attachment 302431 [details] [review]:

fine

Review of attachment 302433 [details] [review]:

I'm pretty sure we can achieve the same result without all the signal juggling by

1) changing the search-changed handler to set_response_sensitive() if we have a selected row and it's mapped

2) set_response_sensitive() on the row-selected handler

Created attachment 302509 [details] [review]
AppChooser: Desensitize ‘Add’ button when no app is selected

I've updated and simplified this as you suggested.

The ‘Add Application’ button shouldn't be sensitive if there is no
application currently selected. This is complicated be the fact that
when Gtk.Listbox filters the list it does not clear the selection so if
there is a selected row we need to check if it is mapped before changing
the state of the ‘Add Application’ button.

Created attachment 302510 [details] [review]
AppChooser: Select app when row is activated

I've updated this to make sure it doesn't add an application when all
the applications are hidden by the filter.

This allows the user to select a startup application by double clicking
on it rather than having to select it and then click on the ‘Add
Application’ button. If there are now rows visible then GtkListBox will
activate the (hidden) selected row so we need to check that the
activated row is mapped.

Created attachment 302511 [details] [review]
AppChooser: Add a search button to header-bar

This gives a visual prompt that it's possible to search the list of
applications.

Created attachment 302513 [details] [review]
AppChooser: Add an accelerator to toggle searching

Use Control-f to toggle the state of the search-bar

Review of attachment 302511 [details] [review]:

looks good

::: gtweak/tweaks/tweak_group_startup.py
@@ +21,3 @@
 import logging
 
+from gi.repository import Gtk, Gdk, GLib, Gio, GObject

The aditional import shouldn't be needed

Review of attachment 302513 [details] [review]:

++

Review of attachment 302510 [details] [review]:

++

Review of attachment 302509 [details] [review]:

nice

(In reply to Rui Matos from comment #26)
> Review of attachment 302511 [details] [review] [review]:
> 
> looks good
> 
> ::: gtweak/tweaks/tweak_group_startup.py
> @@ +21,3 @@
>  import logging
>  
> +from gi.repository import Gtk, Gdk, GLib, Gio, GObject
> 
> The aditional import shouldn't be needed

I get this error if I take it out
    self._binding = searchbtn.bind_property("active", self.searchbar, "search-mode-enabled", GObject.BindingFlags.BIDIRECTIONAL)
NameError: global name 'GObject' is not defined

Attachment 301710 [details] pushed as f78155a - StartUpTweak: Use g_application_mark_busy()
Attachment 301711 [details] pushed as 7664aba - AppChooser: Include matching running apps when filtering
Attachment 301714 [details] pushed as 82924af - AppChooser: Add a default response
Attachment 301716 [details] pushed as 019eda9 - AppChooser: Rationalize ‘Escape’ handling
Attachment 301717 [details] pushed as 96014af - AppChooser: Respect setting of gtk-dialogs-use-header
Attachment 302431 [details] pushed as a4f5db2 - AppChooser: Don't show existing start-up apps
Attachment 302509 [details] pushed as 6874ba8 - AppChooser: Desensitize ‘Add’ button when no app is selected
Attachment 302510 [details] pushed as 1372b5f - AppChooser: Select app when row is activated
Attachment 302511 [details] pushed as ebc6b70 - AppChooser: Add a search button to header-bar
Attachment 302513 [details] pushed as c60d424 - AppChooser: Add an accelerator to toggle searching

Are you happy for me to backport the following patches to the 3.16
branch? It would be nice to have the fixes that don't require a Gtk
version bump or string changes in the stable branch. I can just
cherry-pick and push or open a new bug if you want to check them first.

f78155a StartUpTweak: Use g_application_mark_busy()
7664aba AppChooser: Include matching running apps when filtering
a4f5db2 AppChooser: Don't show existing start-up apps
82924af AppChooser: Add a default response
019eda9 AppChooser: Rationalize ‘Escape’ handling

(In reply to Phillip Wood from comment #30)
> I get this error if I take it out
>     self._binding = searchbtn.bind_property("active", self.searchbar,
> "search-mode-enabled", GObject.BindingFlags.BIDIRECTIONAL)
> NameError: global name 'GObject' is not defined

Ah, nevermind, I totally overlooked that last argument.

(In reply to Phillip Wood from comment #31)
> Are you happy for me to backport the following patches to the 3.16
> branch? It would be nice to have the fixes that don't require a Gtk
> version bump or string changes in the stable branch. I can just
> cherry-pick and push or open a new bug if you want to check them first.
> 
> f78155a StartUpTweak: Use g_application_mark_busy()
> 7664aba AppChooser: Include matching running apps when filtering
> a4f5db2 AppChooser: Don't show existing start-up apps
> 82924af AppChooser: Add a default response
> 019eda9 AppChooser: Rationalize ‘Escape’ handling

Yes please, just push them, thanks

Review of attachment 302509 [details] [review]:

::: configure.ac
@@ +10,2 @@
 DESKTOP_SCHEMAS_REQUIRED_VERSION=3.4.0
+GTK_REQUIRED_VERSION=3.14.0

Wait, why are we bumping this? Looks like all the API used here exists in 3.12

(In reply to Phillip Wood from comment #31)
> 6874ba8 - AppChooser: Desensitize ‘Add’ button when no app is selected
> 1372b5f - AppChooser: Select app when row is activated

I think these are ok for 3.16 too

Created attachment 302598 [details] [review]
Revert increase in required Gtk version

In reply to comment 33
> Review of attachment 302509 [details] [review] [review]:
>
> ::: configure.ac
> @@ +10,2 @@
>  DESKTOP_SCHEMAS_REQUIRED_VERSION=3.4.0
> +GTK_REQUIRED_VERSION=3.14.0
>
> Wait, why are we bumping this? Looks like all the API used here
> exists in 3.12

Sorry, I'd somehow got it into my head that we needed 3.14 for the
‘row-activated’ and ‘row-selected’ signals.


6874ba8 AppChooser: Desensitize ‘Add’ button when no app is selected
erroneously increased the required Gtk version number.

Review of attachment 302598 [details] [review]:

push it

Comment on attachment 302598 [details] [review]
Revert increase in required Gtk version

Attachment 302598 [details] pushed as 7e75c2a - Revert increase in required Gtk version

(In reply to Rui Matos from comment #32)
> (In reply to Phillip Wood from comment #31)
> > Are you happy for me to backport the following patches to the 3.16
> > branch? It would be nice to have the fixes that don't require a Gtk
> > version bump or string changes in the stable branch. I can just
> > cherry-pick and push or open a new bug if you want to check them first.
> > 
> > f78155a StartUpTweak: Use g_application_mark_busy()
> > 7664aba AppChooser: Include matching running apps when filtering
> > a4f5db2 AppChooser: Don't show existing start-up apps
> > 82924af AppChooser: Add a default response
> > 019eda9 AppChooser: Rationalize ‘Escape’ handling
> 
> Yes please, just push them, thanks

> (In reply to Phillip Wood from comment #31)
> > 6874ba8 - AppChooser: Desensitize ‘Add’ button when no app is selected
> > 1372b5f - AppChooser: Select app when row is activated
>
> I think these are ok for 3.16 too

I've removed the version bump and pushed them