I guess it's because it's just a mock-up, but it gets capitalisation wrong, missing colons after the labels, and has no mnemonics…

The existing dialogue takes great pain to
* look neat: left justified labels, correct capitalisation, colon at the end, correct alignments, spacings, paddings
* accessible: labels have mnemonics and correctly set-up mnemonic targets

Any redesign must preserve these.

Also, I think a new dialogue should be written from scratch instead of trying to change the .ui file for the current one (glade is just SOOOO unusable!), since it's full of using deprecated widgets like [vh]box, table.


Now let's take on the mockup tab by tab:

* 'general' tab:

- not sure that 'copy ID' is recognisable for what it does

- initial size: currently it's controlled by a checkbox, and if not enabled the size is taken from termcap. Since vte only has one builtin termcap, I guess it's ok to just have this setting directly. Default should be 80x24.

- custom font: Do we need to have this controlled by a checkbox? afaik there's no way to set the default monospace font from the control centre, so maybe just have this as an always-enabled setting?

- terminal bell: NO WAY: I won't accept any GtkSwitch widgets being used in g-t. Should be a checkbox like all the others.

* 'title & command'

- the inline label's not really correct. Do we need this inline help at all? The title set there is a 'static' title that's really only used until the first time the terminal application issues the set-window-title escape sequence... maybe just remove it altogehter?

- run as login shell: seems broken currently (not logged to lastlog)… maybe replace this and the 'custom command' setting with a triple radio:
o Shell
o Login shell
o Custom command: [    ]
?

* 'colours'

- the 'bold text' setting was added at user request, not sure we can just remove it again…

- do we need palette customisation at all? In any case, can't have '[builtin name] *and* customised' at the same time; either it's using a builtin palette or a custom one.

- the inline help text is again not very useful.

* 'scrolling'

- the scrollbar setting already has been changed to only be a checkbox 'use scrollbar'.

- 'unlimited' and 'scrollback size' really are either-or, maybe use a radio group?

* 'compatibility'

- there have just been added 2 more settings

- the 'word chars' setting could just be removed and vte changed to use \w regex instead.

(In reply to comment #1)

> * 'colours'
> 
> - the 'bold text' setting was added at user request, not sure we can just
> remove it again…

The 'allow bold text' setting checkbox is still there in 'general'. Or were you talking about something else?

Bold text colour.

(In reply to comment #1)

> - the 'word chars' setting could just be removed and vte changed to use \w
> regex instead.

I filed bug 727743 for this one.

Created attachment 273706 [details] [review]
profile: editor: Remove the word-chars setting

Comment on attachment 273706 [details] [review]
profile: editor: Remove the word-chars setting

Remove the key from the .gschema.xml file too.

Created attachment 273713 [details] [review]
profile: editor: Remove the word-chars setting

(In reply to comment #1)
> I guess it's because it's just a mock-up, but it gets capitalisation wrong,
> missing colons after the labels, and has no mnemonics…

OK, I've tidied up the capitalisation and colons. We can easily add mnemonics once the rest of the design has stabilised.

> The existing dialogue takes great pain to
> * look neat: left justified labels, correct capitalisation, colon at the end,
> correct alignments, spacings, paddings

Left justification isn't always the neatest solution (I have some notes on that here: https://wiki.gnome.org/Design/HIG/Layout#Groups_of_Controls ), that said, I don't feel that strongly on this occasion and have updated the mockup to use left justification.

...
> Now let's take on the mockup tab by tab:
> 
> * 'general' tab:
> 
> - not sure that 'copy ID' is recognisable for what it does

Do you think a different label would make it clearer? Since I don't use this feature I'd appreciate some advice on terminology.

...
> - custom font: Do we need to have this controlled by a checkbox? afaik there's
> no way to set the default monospace font from the control centre, so maybe just
> have this as an always-enabled setting?

I think there should be a way to restore the default, in case you change it and can't remember what the default is. The check box works for this and is visually tidy with the items above.

> - terminal bell: NO WAY: I won't accept any GtkSwitch widgets being used in
> g-t. Should be a checkbox like all the others.

I'd like to discuss this with you; the switch works well here in my opinion.

> * 'title & command'
> 
> - the inline label's not really correct. Do we need this inline help at all?
> The title set there is a 'static' title that's really only used until the first
> time the terminal application issues the set-window-title escape sequence...
> maybe just remove it altogehter?

I felt that the purpose of the setting on its own wasn't clear, and the explanation in the mockup was intended as a rough starting point - I'd be perfectly happy to change the text. Removing it is the other option of course; if it doesn't serve a good purpose I'd be happy with that - up to you.

> - run as login shell: seems broken currently (not logged to lastlog)… maybe
> replace this and the 'custom command' setting with a triple radio:
> o Shell
> o Login shell
> o Custom command: [    ]
> ?

Radios make sense here, although I'm not sure "login shell" will be meaningful. Perhaps something like:

o Shell

  [ ] Update login records when shells are launched

o Custom command: [    ]

> * 'colours'
> 
> - the 'bold text' setting was added at user request, not sure we can just
> remove it again…

I moved the bold text setting to the text appearance section in the general tab - since bold text isn't related to colour.

> - do we need palette customisation at all?

I'd be surprised if many people use it, although there's bound to be someone out there. I was just aiming to preserve existing settings.

> In any case, can't have '[builtin
> name] *and* customised' at the same time; either it's using a builtin palette
> or a custom one.

Most custom palettes will be a mix of a predefined palette and a custom one. I think it's important to indicate which predefined palette a custom one is based on, since in many cases only one or two colours will be changed. The palette could be 90% Tango but you'd have no idea.

Also, the current UI changes the unmodified custom palette depending on the previously selected palette - this is non-obvious and it took me some trial and error to figure out where the pre-defined state of the custom palette came from. There's also the oddity of there being two ways to start a custom palette - either by clicking the colour buttons or selecting custom from the drop down.

> - the inline help text is again not very useful.

The intention of the help text was to explain that not all commands use the palette. I have been confused by this in the past - I'd change the palette and see no change in the Terminal output, and I thought it was a bug. Don't mind removing it or changing it though, if you want.

> * 'scrolling'
> 
> - the scrollbar setting already has been changed to only be a checkbox 'use
> scrollbar'.

OK, I've updated the mockup for that.

> - 'unlimited' and 'scrollback size' really are either-or, maybe use a radio
> group?

The idea was that scrollback size would be insensitive if "unlimited" is checked. I've added a note about this to the mockup - let me know if it's OK.

> * 'compatibility'
> 
> - there have just been added 2 more settings

I don't have an up to date build right now; I assume that the extra settings can be incorporated with the mockup; otherwise let me know what the new settings are and I'll add them.
 
> - the 'word chars' setting could just be removed and vte changed to use \w
> regex instead.

Would regex need to be present in the profile preferences?

--

Updated mockups can be found at the original URL: https://raw.github.com/gnome-design-team/gnome-mockups/master/terminal/terminal-preferences.png

Note that the new mockups assume that the settings for terminal titles and word selection will be removed. As a result, the tabs have been reorganised slightly.

(In reply to comment #8)
> > - the 'word chars' setting could just be removed and vte changed to use \w
> > regex instead.
> 
> Would regex need to be present in the profile preferences?

Christian meant that we should get rid of the settings and have it built into vte. It has now been removed.

Created attachment 274106 [details] [review]
Remove unused code to create terminals with custom titles

Review of attachment 274106 [details] [review]:

chpe OKed this in #vte on GIMPNet:

13:46 <rishi> And then one on bug 724110
13:46 <Services> Bug http://bugzilla.gnome.org/show_bug.cgi?id=724110 normal,   
      Normal, gnome-3-14, gnome-terminal-maint, NEW, Tidy up profile            
      preferences
13:49 <chpe> the 'tidy' patch is ok to commit

Created attachment 274455 [details] [review]
Remove the action and shortcut to set a static title from the menus

Created attachment 274458 [details] [review]
Remove the static title setting from profile preferences

Review of attachment 274458 [details] [review]:

::: src/terminal-screen.c
@@ -729,1 @@
 

I think priv->title is entirely unused with this and the preceding patches, so it can be removed.

(In reply to comment #14)
> Review of attachment 274458 [details] [review]:
> 
> ::: src/terminal-screen.c
> @@ -729,1 @@
> 
> 
> I think priv->title is entirely unused with this and the preceding patches, so
> it can be removed.

The whole description and user_title API code can be removed now. I was going to do it in a separate patch, to keep the changes small. I can squash them into one patch too if you prefer it that way.

Created attachment 274469 [details] [review]
screen: Remove unused description and user_title API

(In reply to comment #8)
> > * 'title & command'
> > 
> > - the inline label's not really correct. Do we need this inline help at all?
> > The title set there is a 'static' title that's really only used until the first
> > time the terminal application issues the set-window-title escape sequence...
> > maybe just remove it altogehter?
> 
> I felt that the purpose of the setting on its own wasn't clear, and the
> explanation in the mockup was intended as a rough starting point - I'd be
> perfectly happy to change the text. Removing it is the other option of course;
> if it doesn't serve a good purpose I'd be happy with that - up to you.

The static title setting has now been removed.

Created attachment 274594 [details] [review]
profile: Change the size UI according to latest designs

Created attachment 274600 [details]
Screenshot of the new size UI

Created attachment 274605 [details] [review]
profile: Change the cursor shape UI according to latest designs

Created attachment 274606 [details]
Screenshot of the new cursor UI

Comment on attachment 274605 [details] [review]
profile: Change the cursor shape UI according to latest designs

Not sure about this one. Aligning the right end of the combo with the 'rows' entry above doesn't feel natural, esp since the control above actually is '[ ] cols [ ] rows' ie includes the label...

Since 'title' is gone, should rename the 2nd tab to just 'command'…

(In reply to comment #21)
> Created an attachment (id=274606) [details]
> Screenshot of the new cursor UI

Looks like a great start. There are some alignment issues that need tightening up:

 * Need to increase the vertical padding between the rows in the upper section - some elements look too closely related to the controls above and below.

 * "columns" and "rows" are slightly too far from the spin buttons.

 * Putting the font setting on one row (like in the mockup [1]) would use the space more effectively.

 * Can we group the three text settings together, with a heading? (Also like in the mockup.) This will give some much needed structure to this tab. Terminal bell can just go below cursor shape for now.

 * I agree with Christian about the cursor shape combobox - maybe try extending the width so it is the same length as profile name.

[1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/terminal/terminal-preferences.png

*** Bug 660711 has been marked as a duplicate of this bug. ***

(In reply to comment #24)
>  * "columns" and "rows" are slightly too far from the spin buttons.

Brought them a bit closer to the spin buttons.

>  * I agree with Christian about the cursor shape combobox - maybe try extending
> the width so it is the same length as profile name.

Done.

Created attachment 275327 [details] [review]
profile: Change the cursor shape UI according to latest designs

Created attachment 275331 [details]
Screenshot of the new cursor UI

Created attachment 275341 [details] [review]
profile: Increase the vertical spacing among the rows

Review of attachment 275327 [details] [review]:

This was OKed by chpe in #vte on GIMPNet:

13:33 <rishi> chpe: Any comments on bug 724110 ? Just trying to get the         
      patches in one by one so that I don't have to resolve merge conflicts     
      due to rebasing the XML patches.

15:53 <chpe> rishi: well, ok to commit the prefs patch, but I do think the      
      combo is ridiculously wide for these short strings...

Created attachment 275631 [details] [review]
profile: Move the terminal bell above the text settings

Comment on attachment 275631 [details] [review]
profile: Move the terminal bell above the text settings

Need to fix the 'position' property of the elements following the place where you removed the checkbutton from, or the file will display wrong in glade.

(In reply to comment #32)
> (From update of attachment 275631 [details] [review])
> Need to fix the 'position' property of the elements following the place where
> you removed the checkbutton from, or the file will display wrong in glade.

Hmm... is that so? I have glade-3.16.1 and at appears to be rendering as expected.

According to the XML definition:
  0 - table
  1 - system-font-checkbutton
  2 - alignment containing font-hbox
  3 - allow-bold-checkbutton
  4 - bell-checkbutton (used to be here before being moved to the table)
 10 - rewrap on resize-checkbutton

Looks like glade is able to handle the gap between 4 and 10, but then changing 10 to 5 is trivial, so I will just do that. :)

Created attachment 275632 [details] [review]
profile: Move the terminal bell above the text settings

Created attachment 275648 [details] [review]
profile: Change the font selection UI according to latest designs

Created attachment 275649 [details] [review]
profile: Move the bold and wrap settings above the font button

Created attachment 275650 [details] [review]
profile: Add a heading for the text settings

Created attachment 275651 [details] [review]
profile: Increase the vertical spacing in the upper rows

Created attachment 275652 [details]
Screenshot of the General tab

As far as I can make out there is no way to set the xalign of the font_label widget in GtkFontButton from XML. We either need a GTK+ API or we have to write code on our end.

Created attachment 279612 [details] [review]
profile: Move the terminal bell above the text settings

Rebased on top of master. No idea how this happened.

Created attachment 279615 [details]
Screenshot without increase in vertical spacing

Created attachment 279617 [details] [review]
profile: Remove empty space at the top of the General tab

Created attachment 279751 [details] [review]
profile: Change the font selection UI according to latest designs

Created attachment 279752 [details] [review]
profile: Move the bold and wrap settings above the font button

Created attachment 279753 [details] [review]
profile: Add a heading for the text settings

Created attachment 279754 [details] [review]
profile: Remove empty space at the top of the General tab

Created attachment 279755 [details] [review]
profile: Add some space above the text appearance section

Created attachment 279756 [details] [review]
profile: Increase the vertical spacing in the upper rows

I assume the look of the latest patches is like https://bug724110.bugzilla-attachments.gnome.org/attachment.cgi?id=279615 except that the empty space at the top was removed?

Attachment 279756 [details] looks doubly wrong: it uses margins which is not nice, and it increases padding where no padding should increase. Drop this bit, please.

Other nits to fix:
* There needs to be a 18px spacing between "[ ] Terminal bell" and the "Text appearence" header.
* There needs to be a 12px left indent for the items under the "Text appearance" header.
* The "columns" and "rows" labels need to be normal, not dim.

I assume you've checked that all labels still have mnemonics that show up on Alt press, and that activate the correct mnemonic targets?

(In reply to comment #50)
> Attachment 279756 [details] looks doubly wrong: it uses margins which is not nice, and it
> increases padding where no padding should increase. Drop this bit, please.

Done.

> Other nits to fix:
> * There needs to be a 18px spacing between "[ ] Terminal bell" and the "Text
> appearence" header.

Done.

> * There needs to be a 12px left indent for the items under the "Text
> appearance" header.

Done.

> * The "columns" and "rows" labels need to be normal, not dim.

Done.

Created attachment 280143 [details]
Screenshot of the General tab

Created attachment 280144 [details] [review]
profile: Move the terminal bell above the text settings

Created attachment 280145 [details] [review]
profile: Change the font selection UI according to latest designs

Created attachment 280146 [details] [review]
profile: Move the bold and wrap settings above the font button

Created attachment 280147 [details] [review]
profile: Remove empty space at the top of the General tab

Created attachment 280148 [details] [review]
profile: Don't dim the columns and rows labels

Created attachment 280149 [details] [review]
profile: Add a mnemonic for resetting the size

Created attachment 280152 [details] [review]
profile: Move the bold and wrap settings above the font button

Made a mistake during interactively rebasing and squashing.

Created attachment 280153 [details] [review]
profile: Add a heading for the text settings

These were ACKed by chpe in #vte on GIMPNet:
12:27 <rishi> chpe: Attached a screenshot after addressing your comments.
12:29 <chpe> that looks fine, thanks!

Created attachment 280155 [details] [review]
profile: Move the ID to the second line

Created attachment 280254 [details] [review]
profile: Change the scrollback UI according to latest designs

Created attachment 280255 [details] [review]
profile: Remove useless placeholders and fix the layout

Created attachment 280257 [details] [review]
profile: Remove useless placeholders and fix the layout

Created attachment 280258 [details]
Screenshot of the Scrolling tab

These were ACKed by chpe in #vte on GIMPNet:
12:42 <rishi> chpe: Do you like the scrolling patches?
12:43 <chpe> yes, they're fine

Can I ask why --title was removed?  Was it causing a problem?

Yes, I understand I can use escape sequences to set titles but that is a lot messier than than just being able to do:

$ gnome-terminal --title linux -e "\"screen -x || screen\"

For example.  Now I have to embed escape sequences into all of my command window start ups.

Is my mom (OK, not mom, but equally so a not-a-programmer-user) supposed to know about escape sequences and how to stack/embed them into a terminal startup sequence?  She will remember aptly named command line options much better than she will remember escape sequences.  So will I for that matter.

I would add to my previous comment that requirement of having to echo escape sequences means that every "-e command" gnome-terminal option now has to spawn a shell around the command you would otherwise normally just want to run such as:

$ gnome-terminal -e "bash -c \"echo -en \\\"\033]0;Top\007\\\"; top\""

instead of the previously too simple:

$ gnome-terminal --title Top -e "top"

Could I get rid of the extra bash?  Sure with:

$ gnome-terminal -e "bash -c \"echo -en \\\"\033]0;Top\007\\\"; exec top\""

But is all of this bash trickery with exec and echoing escape sequences and a trip to quote-escaping hell really progress?

Was the simplicity of the "--title" option really so bad to require this alternative?

@j.e.labarre: Personal attacks are not acceptable. Read and follow https://wiki.gnome.org/Foundation/CodeOfConduct if you'd like to participate in GNOME spaces. Thanks.