Fixing this would also resolve bug 95352.

+1 for making it happen, it's really cool :)

Tilix's Preferences dialog works like this, and IMO it's much better than gnome-terminal's.

Still a giant +1 from me for implementing this. (Not sure if I'll have time+motivation do to it myself, though.)

Just one UI dilemma I have:

In the current mockup, if you pick a non-profile in the left bar (General, Shortcuts, Encodings) then the horizontal tab bar of the right panel (General, Title & Command, Colors...) is gone.

Would it be any better to have only a single non-profile entry in the left bar ("Global prefs") and its subsections (General, Shortcuts, Encodings) laid out horizontally as tabs in the right panel?

There's plenty of room at both places (it's the profile settings that are more crowded) so it's probably not about which one looks better (or if it is, then in the sense of not having too much empty space), but about which one's more intuitive to use.

I'm really not sure.

(By the way, the Encodings page was dropped, so currently it's only two global pages: General and Shortcuts that we have.)

I was thinking more about this feature...

Probably the most frequently used desktop apps are web browsers. I can only check Firefox and Chrome now, but both of them stopped having a settings dialog using a native widget set like GTK+ (maybe Chrome never did). Instead they use their own rendering, that is, the settings are also HTML pages that open as regular tabs of these browsers.

Following this pattern, I'm wondering whether we should ditch our current approach, and use our own interface, that is, VTE for preferences. GNOME Terminal would open another VTE tab, and inside that there would be a nice, user friendly application (probably written in ncurses) offering the functionality of the current settings dialogs.

I this would be a great compromise between being cool geeky and still user friendly. What do you think?

Created attachment 366299 [details] [review]
v0.0

Initial demo version.

GTK+, of course. Stupid U.S. versus rest of the world date formats, I must have mixed up 1/4 with 4/1. I was of course kidding with ncurses.

Don't look at the patch yet, the code is awful. Look at the new UI instead, it's quite promising :)

Most of the functionality is already there in the new single merged prefs window. You can modify the global and per-profile prefs. Meta-profile operations (new/clone/delete profile) are still missing.

I'll post at least two additional comments soon, one about technical stuff, another about UI.

There's still too much work left to do, I don't want to rush and I don't want to squeeze in such a big UI change just before the UI freeze. So let's target the autumn release.

Technical: per-profile notebooks:

It was a real pain to implement the per-profile settings bits in a way that the usual notebook (General Command Colors Scrolling Compatibility) is available for all the prefs. (Just as much as it's apparently a bit painful in dconf too, at least dconf-editor for example doesn't know about the schema...)

The attached patch is my third attempt, and I'm not entirely certain that is the best of the three variants I've played with, nor that there isn't another even better approach.

Approach 1:

I had only one notebook, completely merged the two .ui files (merged profile-preferences.ui into preferences.ui), and eventually probably would have merged profile-editor.c with terminal-prefs.c too.

The big problem is: There are plenty of signals and plenty of gsettings bindings set up, and they need to be unset all before switching to another profile. There doesn't seem to be an existing method (nor seems to be doable using the existing methods) to walk through all the children widgets and wipe out all the signals and bindings they have, nor to change the schema where an existing gsettings binds to. I even run into troubles where gsettings' doc says "you can have only one binding per object property. If you bind the same property twice on the same object, the second binding overrides the first one." but it forgets to say that without an explicit unbind, a rebinding preserves the state of the UI box and updates the settings underneath. Long story short, I had dozens of iterations where settings were leaking from one profile to another as you switched on the UI.

The way I could fix it was to have wrappers like profile_prefs_signal_connect() and profile_prefs_settings_bind() etc. that call their g_ counterparts and also do some bookkeeping: append their parameters to some array, and when switching away from the profile walk through that array to disconnect/unbind them all. I wasn't fully happy with this approach, and was a bit afraid that even after careful disconnecting/unbinding, there might be corner cases (or a faulty future change introduced) that would cause setting leakage across profiles.

Approach 2:

Keep the two .ui files separate, keep two GtkBuilders, and attach the notebook to the corresponding place in the other. Set up all the signals and bindings as currently done. When switching profiles, just gtk_widget_destroy() the entire subtree of the notebook (that should take care of freeing up all the signals and bindings), and reattach it again from a new builder.

For some mysterious reason, I couldn't perfectly implement this. Even after adding the bookkeeping and explicit disconnect/unbind stuff from approach 1, playing with the color pickers soon caused GTK+ warnings of objects not being of the desired type, and eventually a crash. Looked like some memory management problem, use-after-free or alike. I couldn't easily find the culprit and just moved on to approach 3.

Approach 3 (currently implemented):

Keep the two separate .ui files (as in approach 2), and immediately create a prefs editor notebook for each of the profiles.

First I was worried that it would complain about clashing "id"s, but it doesn't. Seems that GTK+ doesn't even have the concept of widget ID (correct?), it's just a thing of the glade (xml) files.

One drawback of this approach is that setting up the prefs dialog might get a bit slow. With all these signals connected and gsettings bindings and whatnot, initializing such a profile prefs notebook takes about 30ms on my computer (Core i5 @ 2300 GHz), that is, 30 profiles per second. Someone with a noticeably slower computer and 5 or so profiles might easily notice this delay. However, there's probably hardly anyone having more than 5 profiles, so it's not a big deal. Also, memory consumpiton is also sure affected.

Also, for the same reason, if the list of the profiles changes (perhaps externally), we probably shouldn't just throw out all of them and rebuild them, but should keep the ones we can keep.

This approach also has the advantage that in the weird case that these notebooks aren't all equally sized (e.g. after a future layout reorganization, the biggest tab has a text entry field which contains some bigger Unicode glyph in one of the profiles), the overall window size is automatically adjusted to the biggest, rather than jumping as you switch the profile in the sidebar.

I have a bit of a special code to keep the notebooks of the various profiles all switched to the same tab, so that one can easily walk across all the profiles and see whether let's say anything under the Compatibility option changes.

I tend to be the most satisfied with this approach so far, although I'm happy to switch to another one if there's a compelling reason.

Technical: sidebar:

Currently the sidebar is a ListBox. It was tempting to use TreeView (as is done currently in the Profiles tab of the global Preferences dialog), I could've even reused more of the current code.

One thing is that as you start adding icons and stuff, it doesn't look that good / that flexible as other solutions such as ListBox.

Another, deal breaker was that test case from bug 767468 (fixed, continued in another bug, reopened, fix in some other way...) didn't work for me, so it seems you cannot have a popover opening from a treeview. And even if the patches fixed it as they claim (who knows, maybe it's broken for me downstream due to an Ubuntu patch?), we'd have to significantly bump the GTK+ requirement.

Created attachment 366304 [details]
v0.0 screenshot

UI issues/questions in random order:

- Should there be a "Global" label in the upper left or not?

- Somehow get rid of the word "General" used at two places.

- Equal vertical spacing in the sidebar.

- Should all the profiles have their triangle (for the popover) visible (as in Tilix), or just the currently selected (as in the mockup and screenshot)?

- How to rename profiles? I think I'd add it to the popover (as per the mockup) and remove it from the right-hand side.

- Alphabetical sorting of profiles is not yet implemented, and its implementation would depend on the answer to the previous question. If we keep the current "Profile name" entry box, I wouldn't re-sort them, that'd look silly during typing. If we only have a dedicated popover entry with an explicit Enter keypress to accept the value, I'm okay with re-sorting. Or perhaps allow the users to manually drag-n-drop and hence specify their preferred order of the profiles?

- Move Profile ID somewhere, maybe to the bottom right corner? Or don't show it, just add an option to the popover to copy that to the clipboard?

- Where to put the "Add profile" (or "New profile") action? The bottom left corner, as per the mockup, probably wouldn't look nice along with the Help button. Or to the popover (technically not logical there but probably perfectly usable)? Or have a popover triangle next to the "Profiles" label for this?

- Keyboard navigation (up/down arrows) sucks in the sidebar, they don't jump over the Global/Profiles labels (not even if I make them inactive). Okay to live with this? Any workaround? Use two listboxes maybe, or some different widget?

- Shall we keep the two menu entries Edit -> Preferences and Edit -> Profile Preferences? The only difference now is which entry they select from the sidebar. They can have corresponding shortcuts, and it would be nice to keep the shortcut for editing the _current_ profile. Most of the settings are per-profile anyway, and it's always easier to go in the settings dialog from the current profile to General, rather than from General to... "wait, which profile was I just using?" We could keep both menu entries, or perhaps just have one that selects the current profile (works as Edit -> Profile Preferences) even though renamed to the shorter form?

- I'd probably remove the File -> New Profile menu entry.

- Adwaita seems to have white background for notebook pages, and gray for outside-of-notebook. As a result, general prefs used to have white background, but now it has gray. It looks silly as you switch between a profile and the General page, and the background color keeps changing. Maybe add an otherwise pointless notebook? (Another reason for me not to like Adwaita.)

- There's hardly anything on the General page, and even half of that is related to shortcuts, so why not move them over there? (Question applies to current g-t too.) Or we could merge these two pages to one. Or have new options for General, such as whether to always show the notebook over terminals (I'd even prefer to make this the default by the way), or whether tabs are on top or bottom, or a few others we have pending feature requests for.

If it helps, Tilix does Approach 1 where it uses a single UI for profiles and then binds/unbinds settings on the fly as the user selects the profile on the left. It uses a wrapper to manage all of the binds similar to the direction you outlined:

https://github.com/gnunn1/tilix/blob/master/source/gx/gtk/settings.d

I was concerned about the memory usage of duplicating the controls for every profile. I think i Tilix there is a tendency to have more profiles since it supports switching between profiles automatically based on context (server, directory and user).

Thanks for the input :)

I actually kept launching Tilix with the Gtk inspector to see what widgets it has on the left, and kinda copied them. It did occur to me to check how it does the profile change on the right hand side, but intentionally decided not to (at least for now) to see what I can come up with :)

Will be curious to hear Christian's opinion.

(In reply to Gerald Nunn from comment #10)

> I was concerned about the memory usage of duplicating the controls for every
> profile.

With my current patch (approach 3) it's ~1.2 MB per profile (including the UI, signals and bindings).

> - Keyboard navigation (up/down arrows) sucks in the sidebar, they don't jump
> over the Global/Profiles labels (not even if I make them inactive).

I've figured out, the right approach is not to have this as a fixed label as GtkListBoxRow, but rather dynamically add it as a row header to whichever profile is listed first (see gtk_list_box_set_header_func() and friends). This is the semantically correct approach according to the docs.

This fixes the up/down arrow navigation. (Pgup/pgdn arrow navigation still jumps by a pseudo-random amount, while works as home/end in Tilix. No clue about it.)

(In reply to Egmont Koblinger from comment #8)

> Currently the sidebar is a ListBox. It was tempting to use TreeView (as is
> done currently in the Profiles tab of the global Preferences dialog), I
> could've even reused more of the current code.

Especially that I've (mostly) figured out the keyboard nav issue with ListBox, this seems to be the best widget choice for the sidebar to me.

I'd love to keep the use of an underlying model rather than ad-hoc creation of the widget. For some time I was totally happy to see that the current treeview uses a GtkListStore as model, whereas the new listbox can also use a GListStore model.

Then, after looking at it I-don't-know-how-many times, I finally realized that pretty confusingly "Gtk"ListStore != "G"ListStore, not even a derived class, they're totally independent.

I have no idea why GTK+ folks decided to come up with a brand new, incompatible model structure rather than reusing the well-established one.

Anyway, I still think it'd be a nice idea to have a model, so I'm planning to go in this direction. Even if it's a "G"ListStore. Even if it requires to slightly bump our glib/gtk+ version requirement (to 3.5 years old version by the time I'm planning to ship the redesign, so I'm sure that's okay).

I talked to Christian off-bugtracker and he prefers the Approaches in decreasing order of 1, 2, 3. So the winner is approach 1.

I wasted quite some time on 2 & 3, but gained precious experience, so it was worth it :)

Created attachment 366485 [details] [review]
v0.1

Just a checkpoint / backup.

Switched to "Approach 1". Use a "G"ListStore under the sidebar's ListBox, and use the header_func feature to display the "Global" and "Profiles" labels.

Still no meta-profile operations.

Most of the code is still a mess, don't look at it :-D

As said earlier, the use of header_func fixes the navigation in the sidebar using Up/Down arrows.

PgUp/PgDown are more problematic (I guess I should file bugreports). Interestingly, Tilix doesnt' suffer from these problems.

- Downwards they jump in steps of 2, upwards in steps of 3.

- Unlike with Up/Down, the row headers aren't skipped, they count as one during these jumps.

- If a PgUp/PgDown move would end on a row header, it keeps going all the way to the very end in that direction.

- It's easy to hit crashing (and just recently fixed) overrun at the top: bug 791549.

Created attachment 366762 [details] [review]
v0.2

Just another checkpoint.

Meta-profile operations (incl. rename) now available in the sidebar. (The UI is not fully updated after them, though.) Leftovers from the old "Profiles" tab (UI and code) removed.
 
Correspondingly, Profile Name entry field removed from the General tab. Profile ID moved to the bottom right, not sure about it.

There are some stale widgets; if you open and close the prefs dialog and then modify the settings externally, don't be surprised if it crashes.

The code is still in draft state.

---

Coming up next:

I'll stop using a model (that certain "G"ListStore) behind the listbox, and just manipulate the listbox rows directly.

GListStore is way too simple (supports adding/removing rows, but cannot propagate changes of a model row to the view), and as such, hardly helps me, just puts obstacles in the way. There's no mapping between model rows and listbox rows maintained (and exposed) by GTK+, and I'd have to maintain it in at least one direction (the model needs access to the profile name to sort lexicographically, the view needs access to it to display it), but most likely in both directions; or copy all the data from the model row to the view row upon creation and just leave the model row behind (in which case there's no more point in having a model at all) or sync between the model and the view in two directions (not much point either). I can't see how it could be done any simpler/cleaner than with just the ListBoxRows directly.

Created attachment 366804 [details] [review]
v0.3

Fully functional, including meta-profile operations (new, clone, rename, delete, make default).

Hopefully no longer crashes (the builder wasn't unreffed which resulted in leftover bindings/signals to stale combobox models and such).

The code isn't a terrible mess anymore, but still needs lots of cleanup (and some smaller functional fixes too).

Created attachment 366806 [details]
v0.3 screenshot

Here's how it looks like now, including the popover dropdown.

Is it obvious what the "home" icon stands for? By the way, it even has a tooltip text saying "This is the default profile".

In the dropdown of the default profile, the entries "Delete" and "Make default" are insensitive (gray), I'm wondering again if it's obvious why, e.g. what to do if someone wishes to delete the current default.

Renaming cries to be available from the sidebar, so I removed the label from the top of the "General" notebook tab. The Profile ID looked stupid at the new top position. Not sure about the corner where it is now. At some point I removed it and made a "Copy ID" menu entry in the dropdown, but I'm afraid that's absolutely not self explanatory. If the "Compatibility" tab was called "Advanced", I'd move it there. (See also bug 731233.)

"New" is not logical to be under this dropdown, but probably it's the nicest look we can have and the behavior is obvious.

Created attachment 366807 [details]
v0.3 screenshot 2 (renaming)

This screenshot shows a profile getting renamed. It's currently inlined in the sidebar (the label becomes an entry field).

Technically it'd be easier to have a popup dialog window instead, and I'm considering it not for the sake of rename itself, but for the sake of new/clone operations.

Currently new/clone just create a new profile (named "Unnamed" or having the same name as the reference) and nothing more happens. It's a shortcoming of the current patch that it does not select the new entry in the left bar, I'll add it. I guess it should automatically switch to the label getting edited too, since probably that's the very first thing one would want to do with a new profile.

I don't like the current approach of giving the new profile the name "Unnamed" or the same name as the reference, I think it should go like "New" or if that name is taken then "New 2" etc., or "Foo (copy)", "Foo (copy 2") etc. after cloning "Foo". Having the exact same name also means that the sorting order (e.g. above or below the clone reference) is unspecified.

And then when the user gives a name to this new profile, it jumps in the sidebar according to its new lexicographical position.

I'm wondering if it's a better user experience to present a popup dialog asking for the new name, and for the new/clone operation to happen only when the new name is entered. And if we do so, it would be more consistent for a subsequent rename operation to happen in a popup dialog window, too.

(In reply to Egmont Koblinger from comment #20)
> Created attachment 366806 [details]
> Is it obvious what the "home" icon stands for? 

Not at all. Maybe a checkmark would work better?

> By the way, it even has a
> tooltip text saying "This is the default profile".
> 
> In the dropdown of the default profile, the entries "Delete" and "Make
> default" are insensitive (gray), I'm wondering again if it's obvious why,
> e.g. what to do if someone wishes to delete the current default.

If it can be made clear which profile is 'default' then IMHO it's clear that you cannot delete it. So greying out these should be ok.

> Renaming cries to be available from the sidebar, so I removed the label from
> the top of the "General" notebook tab. The Profile ID looked stupid at the
> new top position. Not sure about the corner where it is now. At some point I
> removed it and made a "Copy ID" menu entry in the dropdown, but I'm afraid
> that's absolutely not self explanatory. If the "Compatibility" tab was
> called "Advanced", I'd move it there. (See also bug 731233.)

I like the idea of a "Copy profile ID" in context menu; people have complained about having the ID visible in the UI so that would solve that problem while preserving the reason it's there (to copy it and paste into a command). If you think it's too much of a mystery, maybe only show it when the context menu was invoked while Ctrl was pressed?

> "New" is not logical to be under this dropdown, but probably it's the nicest
> look we can have and the behavior is obvious.

Could be a "+" button on the bold "Profiles" line?

(In reply to Egmont Koblinger from comment #21)
> This screenshot shows a profile getting renamed. It's currently inlined in
> the sidebar (the label becomes an entry field).
> 
> Technically it'd be easier to have a popup dialog window instead, and I'm
> considering it not for the sake of rename itself, but for the sake of
> new/clone operations.

Replacing the label with an entry (and back again on focus out?) seems weird to me. A popover would be nicer, IMHO. Also the entry looks really tiny, so the user might feel pressured into using a short name by the constricted space?

> Currently new/clone just create a new profile (named "Unnamed" or having the
> same name as the reference) and nothing more happens. It's a shortcoming of
> the current patch that it does not select the new entry in the left bar,
> I'll add it.

Yes it should switch to editing the new profile automatically.

> I guess it should automatically switch to the label getting
> edited too, since probably that's the very first thing one would want to do
> with a new profile.

Or perhaps the clone operation could use a popover to provide the new name *before* the cloning operation happens? I see you proposed that below, so take this as me agreeing :-)
 
> I don't like the current approach of giving the new profile the name
> "Unnamed" or the same name as the reference, I think it should go like "New"
> or if that name is taken then "New 2" etc., or "Foo (copy)", "Foo (copy 2")
> etc. after cloning "Foo". 

Yes, that's bug 697787. "Foo (clone)" or "Foo (copy)" would work as default (or, if you go for the above suggestion to first edit the name before cloning, as suggested name in the popover).

>Having the exact same name also means that the
> sorting order (e.g. above or below the clone reference) is unspecified.

Yes, that would confused the user about whether he's about to modify the original by accident. Maybe a "NEW!" badge next to the label in the sidebar for just-cloned profiles (there until the next clone operation, or the prefs are closed) could help here?

> And then when the user gives a name to this new profile, it jumps in the
> sidebar according to its new lexicographical position.

Yes, that's confusing. First providing the name before cloning would also fix this.

> I'm wondering if it's a better user experience to present a popup dialog
> asking for the new name, and for the new/clone operation to happen only when
> the new name is entered. And if we do so, it would be more consistent for a
> subsequent rename operation to happen in a popup dialog window, too.

Yes to both. I'd go for popover instead of popup dialogue, however, with the popover's relative-to pointing to the label of the profile being renamed/cloned.

(In reply to Christian Persch from comment #22)

> > Is it obvious what the "home" icon stands for? 
> 
> Not at all. Maybe a checkmark would work better?

Tilix uses a checkmark. I was thinking like "home" does have a meaning on its own, a checkmark doesn't, it's the chosen _something_, but what something? That's why I went with "home".

Turns out there's an "emblem-default" which is apparently semantically what we're looking for, and it's implemented as a checkmark (although a different one that Tilix's). So I've switched to this, let's see how it looks.

> I like the idea of a "Copy profile ID" in context menu

IMHO this label is already much wider than the others, and still isn't obvious. Like, where does it copy it? I can imagine users wondering what happened after they clicked here. An obvious one ("Copy profile ID to clipboard") is waaay too long.

I've left it in the notebook area for now, we'll see.

All the rest of your feedback should be addressed now in v0.4.

Created attachment 366977 [details] [review]
v0.4

The user experience (especially the behavior of profile meta-operations) should be pretty much final now. Please give it another round of testing :-)

Time to start cleaning up the code...

Popover's doc says "[...] the popover is dismissed in the expected situations (clicks outside the popover, [...])"

In fact, clicking on empty areas or GtkLabels in the right-hand area doesn't dismiss it.

Adding EventBoxes as children of the right-hand side's Stack (which implements the three main pages for the right-hand side pane: General, Shortcuts and Profile prefs) fixes / works around this. My next patch will include this. Interestingly, adding an EventBox outside of the Stack doesn't fix it, so under the Stack it stops listening for events?? This "stopping" doesn't seem to happen with the Notebook of profile prefs.

Bug on Popover?? Bug in Stack?? Bug in Popover's docs and this is the expected behavior??

For the problem of the two "General" labels, here's an idea to consider:

- Rename the profile pref's "General" notebook label to "Text"
- Remove the "Text Appearance" subheader
- Unindent the options below
- Shuffle the lines so that the font options go up, the cursor options go down (see also bug 792794)
- Figure out what to do with the "bell" option, it no longer fits anywhere :P

(In reply to Egmont Koblinger from comment #25)
> Popover's doc says "[...] the popover is dismissed in the expected
> situations (clicks outside the popover, [...])"
> 
> In fact, clicking on empty areas or GtkLabels in the right-hand area doesn't
> dismiss it.

Hmm weird. However that's a corner case, so you shouldn't focus on that, as long as the general interaction works. 

> Adding EventBoxes as children of the right-hand side's Stack (which
> implements the three main pages for the right-hand side pane: General,
> Shortcuts and Profile prefs) fixes / works around this. My next patch will
> include this. Interestingly, adding an EventBox outside of the Stack doesn't
> fix it, so under the Stack it stops listening for events?? This "stopping"
> doesn't seem to happen with the Notebook of profile prefs.
> 
> Bug on Popover?? Bug in Stack?? Bug in Popover's docs and this is the
> expected behavior??

Adding the event box 'fixing' this suggests to me that it's a gtk bug.

(In reply to Egmont Koblinger from comment #26)
> For the problem of the two "General" labels, here's an idea to consider:
> 
> - Rename the profile pref's "General" notebook label to "Text"

or "Appearance" ?

> - Remove the "Text Appearance" subheader

Yes

> - Unindent the options below

Yes

> - Shuffle the lines so that the font options go up, the cursor options go
> down (see also bug 792794)
> - Figure out what to do with the "bell" option, it no longer fits anywhere :P

Could move it to the Compat tab?

(In reply to Christian Persch from comment #27)

> > - Rename the profile pref's "General" notebook label to "Text"
> 
> or "Appearance" ?

The problem with that is that "Color" is also "Appearance".

> > - Figure out what to do with the "bell" option, it no longer fits anywhere :P
> 
> Could move it to the Compat tab?

Maybe... I'm not sure... let's think about it for a few days :)

Created attachment 367257 [details] [review]
v0.4.1

Not much new, just the eventbox, and updated to current master.

Created attachment 367336 [details] [review]
v0.5

The source code is now in reasonably good shape.

All features implemented (the last missing one was: when custom command cannot be launched, open the prefs dialog at that tab).

Feature change: In terminal's right click menu, moved Preferences out of the Profiles submenu.

> > - Figure out what to do with the "bell" option, it no longer fits anywhere

> Could move it to the Compat tab?

If there was a tab called "Other" or "Misc", I'd move it there. Given the current tabs, I'd rather leave it under the new "Text" than move to "Compatibility", for reasons I cannot really explain :) The new "Text" tab would start with options that are closely related to text/font, then move on to cursor properties which are... well... somewhat text-related but not that much, and then finally the Bell option wouldn't be _that_ bad here I think. I don't expect users to complain about it.

That being said, renaming that "General" tab to "Text" is just one possibility, not necessarily the best one.

What I really strongly would like to do, is to get rid of two "General" labels visible at the same time. They'd cause quite some confusion in Help pages, as well as on random internet forums people trying to help out each other with gnome-terminal.

And so far this is my best idea :)

Created attachment 367534 [details] [review]
help update

On top of "first round of fixes" from bug 792529.

Aims to match the current (v0.5) patch, that is, does not yet contain the pending General -> Text rename.

Tons of further cleanup and improvements would be desired for the help pages, I think the current patch brings them to a "mostly good enough" state.

Visually it looks very nice; however I think the left pane should have a frame around it like the right pane does.

Not reviewing the .ui file changes :-)

The code to rebind the UI to a new GSettings is a bit ugly, but getting this right would need glib changes...

As for the rest, just some comments:

+// FIXME copied from terminal-window.c, maybe move to terminal-util.c.

Not necessary, since it's such a tiny function.

+  title = g_strdup_printf (_("GNOME Terminal Preferences – %s"), subtitle);
I don't like this big title; IMHO you should remove the "GNOME Terminal" prefix bit and just be called "Preferences — %s" again.

+listbox_row_selected_cb (GtkListBox *box,
+                         GtkListBoxRow *row,
+                         GtkStack *stack)
+{
+  profile_prefs_unload ();
+
+  /* row can be NULL intermittently during a profile meta operations */
+  g_free (the_pref_data->selected_profile_uuid);
+  if (row != NULL) {
+    the_pref_data->selected_profile = g_object_get_data (G_OBJECT (row), "gsettings");
+    the_pref_data->selected_profile_uuid = g_strdup (g_object_get_data (G_OBJECT (row), "uuid"));
+  } else {
+    the_pref_data->selected_profile = NULL;
+  }

In the |else| branch, selected_profile_uuid is now pointing to freed memory. Set it to NULL.

+    char *rowuuid = g_object_get_data (G_OBJECT (row), "uuid");

const char*

+  // FIXME terminal_app_new_profile() first creates the new profile with empty name,
+  // adds it to the list (which triggers sorting) and then sets the name. So we need to
+  // re-sort. Ideally it should create the new profile with the new name straight away.

terminal-settings-list could use dconf directly to write the new profile's visible-name key under the new path *before* adding the new uuid to its list...

+  gtk_widget_hide (GTK_WIDGET (popover_dialog));  // FIXME popdown if gtk >= 3.22 ?

Yes, with #if GTK_CHECK_VERSION(...).

+  // FIXME can we use GCallback here instead of plain C? Would it make sense?
+  (*fn) (name);

No, GCallback is |void (*)(void)|, so wouldn't fit.

+  gtk_widget_show (GTK_WIDGET (popover_dialog));  // FIXME popup if gtk >= 3.22 ?
Yes; same as above.

+static void
+popover_dialog_notify_text_cb (GtkEntryBuffer *buffer,
+                               GParamSpec     *pspec,
+                               GtkWidget      *ok)

Is there a reason to use the entry's buffer instead of "notify::text" on the entry itself ?

+                        _("Create"),
+                        _("Clone"),
+                        _("Rename"),
+                        _("Delete"),

These need mnemonics.

+  g_menu_append (model_section, _("Clone…"), "win.clone");
+  g_menu_append (model_section, _("Rename…"), "win.rename");
+  g_menu_append (model_section, _("Delete…"), "win.delete");
+  g_menu_append (model_section, _("Set as default"), "win.make-default");

And these as well.


The code dealing with the sidebar is surprisingly ugly, but that's the fault of the list box widget API. So nothing to change there.

Thanks for the review!

> Visually it looks very nice; however I think the left pane should have a
> frame around it like the right pane does.

As far as I know, frames are generally discouraged (am I right?), I only added to the right pane for its two global pages so that they resemble the third page (the profile prefs notebook, with the notebook's implied frame-like look).

In fact, I was a bit considering going the opposite direction: make the left pane totally borderless, as in Tilix or Nautilus's places sidebar. I don't like that though :)

I'll try a few variants and post tiny patches / screenshots to choose the best.

While at it: I was wondering about adding a frame to the popover dialogs. IMHO they don't stand out enough from their context. I wish GTK+ could shade the rest of the window as if it was unfocused.

> Not reviewing the .ui file changes :-)

Luckily I double-checked them recently, turned out I missed to merge in a recent small change during a rebase. :-)

> The code to rebind the UI to a new GSettings is a bit ugly, but getting this
> right would need glib changes...

Or approach 2 as per above... no, I'm not going to go back there.

> As for the rest, just some comments:

I'll address these tomorrow.

(In reply to Egmont Koblinger from comment #34)
> As far as I know, frames are generally discouraged (am I right?), I only
> added to the right pane for its two global pages so that they resemble the
> third page (the profile prefs notebook, with the notebook's implied
> frame-like look).
> 
> In fact, I was a bit considering going the opposite direction: make the left
> pane totally borderless, as in Tilix or Nautilus's places sidebar. I don't
> like that though :)

To me the frameless left pane looks a bit 'broken' by going from white content to light-grey surrounding without a darker border pixel inbetween. Could just be me :-)
 
> While at it: I was wondering about adding a frame to the popover dialogs.
> IMHO they don't stand out enough from their context. I wish GTK+ could shade
> the rest of the window as if it was unfocused.

BTW, could you make the main label wrap itself in the popup ? As is the Clone popup is rather wide due to the long text.

> To me the frameless left pane looks a bit 'broken' by going from white
> content to light-grey surrounding without a darker border pixel inbetween.
> Could just be me :-)

I see, indeed it's not ideal bad with Adwaita. I've tried (for fun) removing the frame from the notebook too, it's more consistent, but looks stupid in Ambiance (and is not what you're looking for either).

IMHO Ambiance looks better without that frame on the left, but I can get used to that. So I'll go with your suggestion.

> BTW, could you make the main label wrap itself in the popup ? As is the
> Clone popup is rather wide due to the long text.

I'd rather find a shorter wording, suggestions welcome :) E.g. "Enter name for new profile based on “foo”:". Maybe "Enter" isn't needed either.

> +                        _("Create"),
> +                        _("Clone"),
> +                        _("Rename"),
> +                        _("Delete"),
> 
> These need mnemonics.
> 
> +  g_menu_append (model_section, _("Clone…"), "win.clone");
> +  g_menu_append (model_section, _("Rename…"), "win.rename");
> +  g_menu_append (model_section, _("Delete…"), "win.delete");
> +  g_menu_append (model_section, _("Set as default"), "win.make-default");
> 
> And these as well.

The surprising thing is... these mnemonics often wouldn't work.

The presence of a popover doesn't disable the rest of the mnemonics in the window, they're still alive. Normally if there's a conflict, pressing the mnemonic cycles through the matches. In this case, however, first the one in the main window is focused, which immediately dismisses the popover.

It's IMO unreasonable to choose the mnemonics so that in none of the pages do they conflict with the popover's entries; and even if we did so, translators couldn't keep it this way.

What we could do:

File a GTK+ bug so that a popover should temporarily disable the mnemonics outside of it;

Figure out what influences the focusing order in case of conflicting mnemonics, and shuffle things around so that the popover's one is focused first. Do you happen to know it? Shuffling the order in the .ui file doesn't help. Not sure if gtk_container_set_focus_chain() is relevant, but looks terribly cumbersome to set up, looks to me that we'd need to manually add all the widgets in the proper order, which is a no-go for me.

We could just live without mnemonics, since (as per bug 792978) they don't work for many of our users anyway. There's no mnemonic for the sidebar's entries and the dropdown arrow either, standard keyboard navigation can be used instead.

For the Cancel / OK buttons ("OK" being "Delete", "Rename" etc.) this doesn't really matter, since the more intuitive Esc / Enter are hooked up as expected.

For the entries of the dropdown menu it would be more important to have these, actually critical for kbd navigation in Ambiance (https://bugs.launchpad.net/ubuntu/+source/ubuntu-themes/+bug/1742269), but it should be fixed by them.

Created attachment 367570 [details] [review]
v0.5.1 (incremental)

On top of v0.5.

> I think the left pane should have a frame around it like the right pane does.

Done.

> +// FIXME copied from terminal-window.c, maybe move to terminal-util.c.
> 
> Not necessary, since it's such a tiny function.

OK, FIXME removed.

> +  title = g_strdup_printf (_("GNOME Terminal Preferences – %s"), subtitle);
> I don't like this big title; IMHO you should remove the "GNOME Terminal"
> prefix bit and just be called "Preferences — %s" again.

Yup, sure – especially given that it's unclear to me whether the user facing name is "Terminal" or "GNOME Terminal". Done.

> In the |else| branch, selected_profile_uuid is now pointing to freed memory.
> Set it to NULL.

Done.

> +    char *rowuuid = g_object_get_data (G_OBJECT (row), "uuid");
> 
> const char*

Done.

> terminal-settings-list could use dconf directly to write the new profile's
> visible-name key under the new path *before* adding the new uuid to its
> list...

This one is still left to do.

> +  gtk_widget_hide (GTK_WIDGET (popover_dialog));  // FIXME popdown if gtk
> >= 3.22 ?
> 
> Yes, with #if GTK_CHECK_VERSION(...).

Not done, added a comment explaining the trouble I ran into.

> No, GCallback is |void (*)(void)|, so wouldn't fit.

Ok, FIXME removed.

> Is there a reason to use the entry's buffer instead of "notify::text" on the
> entry itself ?

Yup, and that reason my lack of experience with GTK+ programming :-) Fixed.

> These need mnemonics.

See the previous comment.

Created attachment 367573 [details] [review]
v0.5.2 (incremental)

On top of v0.5.1.

> +  // FIXME terminal_app_new_profile() first creates the new profile with
> empty name,
> +  // adds it to the list (which triggers sorting) and then sets the name.
> So we need to
> +  // re-sort. Ideally it should create the new profile with the new name
> straight away.
> 
> terminal-settings-list could use dconf directly to write the new profile's
> visible-name key under the new path *before* adding the new uuid to its
> list...

Done. Please take a look, it's quite ugly as I set the name at two places using two different methods (gsettings for "New profile", dconf_changeset for "Clone"). That being said, as per 792990 this area deserves some cleanup anyway.

> +  g_menu_append (model_section, _("Clone…"), "win.clone");

Accidentally introduced mnemonics here in v0.5.1, reverted now.

> > +  gtk_widget_hide (GTK_WIDGET (popover_dialog));  // FIXME popdown if gtk
> > >= 3.22 ?
> > 
> > Yes, with #if GTK_CHECK_VERSION(...).
> 
> Not done, added a comment explaining the trouble I ran into.

Here I was blaming gtk_popover_pop{up,down} for an unrelated bug.

Select a non-default profile, choose to Rename it, press ESC, then delete the profile. Instead of deleting, it gets renamed to ".".

popover_dialog_cancel_clicked_cb() disconnects the handler of the "ok" button, but it doesn't get called when ESC is pressed.

I'll fix it tomorrow, plus move forward with the popover_pop{up,down} stuff.

Created attachment 367660 [details] [review]
v0.5.3 (incremental)

On top of v0.5.2.

Fixes the popover issue discovered yesterday. (Clicking outside the popover also triggered the bug, not just ESC.)

Created attachment 367661 [details] [review]
v0.5.3

An all-in-one (except help pages) version for convenience.

Next time I'll use a wip git branch for such big changes :)

Christian, could you please do another round of review? I'll also do one, hoping for some further tiny cleanups.

> That being said, renaming that "General" tab to "Text" is just one
> possibility, not necessarily the best one.
> 
> What I really strongly would like to do, is to get rid of two "General"
> labels visible at the same time. [...]

Unless there's a better idea, I'll do the "General" -> "Text" rename and the corresponding shuffling of that page as a followup change.

Created attachment 367668 [details] [review]
v0.5.4 (incremental)

On top of v0.5.3.

Have a single popover menu only, taken from the .ui file – rather than one per profile, constructed from C.

I've tried it once, didn't work then (not a surprise given my then-zero experience with popovers). Works this time. The definition was accidentally left in the .ui file, so only minor corrections were necessary now.

This design is more consistent with the popover dialog (clone etc. confirmation). On the other hand, with this approach it wouldn't be possible to have a menu button for all the profiles at once (as Tilix has). I don't think this is a problem, I'd rather not have that design, plus implementing that would require further code changes (we'd have to carry the profile for which the menu entry was invoked, rather than relying on the "global" selected_profile variable.)

> Have a single popover menu only, taken from the .ui file – rather than one
> per profile, constructed from C.

I'm not entirely convinced that this change is a win... happy to abandon (and remove the menu definition from the .ui) if you'd prefer :)

Created attachment 367676 [details] [review]
"Text" tab

Created attachment 367677 [details]
"Text" tab screenshots

Three screenshots in one, showing the "Text" tab with the options reshuffled.

The first is a bit too dense for me, maybe the second with headings is better?

The third shows the heading of the "Colors" tab starting with "Text", pretty much exactly under the unselected "Text" tab label, which doesn't look too good, I mean one can wonder why it's not under the "Text" tab then.

I'm still not entirely sure that this is the right direction... maybe rename the other "General" in the sidebar... to what? "Application" maybe, or "Main"??

v0.5.1 incr: ok

v0.5.2 incr: tiny nit:

+    value = g_variant_new_string (name);
+    if (value) {

value is always != NULL, so no need for this check here.

v0.5.3 incr: ok

v0.5.4 incr: ok

(In reply to Egmont Koblinger from comment #47)
> > Have a single popover menu only, taken from the .ui file – rather than one
> > per profile, constructed from C.
> 
> I'm not entirely convinced that this change is a win... happy to abandon
> (and remove the menu definition from the .ui) if you'd prefer :)

I'm ok with either way.

(In reply to Egmont Koblinger from comment #49)
> Created attachment 367677 [details]
> "Text" tab screenshots
> 
> Three screenshots in one, showing the "Text" tab with the options reshuffled.
> 
> The first is a bit too dense for me, maybe the second with headings is
> better?

It looks a bit more approachable to me, yes.

> The third shows the heading of the "Colors" tab starting with "Text", pretty
> much exactly under the unselected "Text" tab label, which doesn't look too
> good, I mean one can wonder why it's not under the "Text" tab then.

Could use 'Foreground Text and Background colour'?  But it's not a big problem, IMHO.

> I'm still not entirely sure that this is the right direction... maybe rename
> the other "General" in the sidebar... to what? "Application" maybe, or
> "Main"??

"User Interface" maybe?

Created attachment 367839 [details] [review]
v0.5.4.1

All in one patch. Same as 0.5.4, ported to current git (mnemonic changes and such).

Created attachment 367863 [details] [review]
v0.5.5 (incremental)

On top of the previous patch v0.5.4.1.

Just a couple of random cleanups, nothing interesting.

Christian, is there any pending issue with the big patch? I can't recall any.

No, I think this is good to go. Thanks :-)

> No, I think this is good to go. Thanks :-)

You're welcome! It was a great experience :-)

Committing in a few minutes along with the Help updates and the undup of General, I'll go for the "Text" tab label as per the middle screenshot from comment 49.

Followup complaints, improvement requests etc. should go to new bugs.