GNOME Bugzilla – Bug 791968
UI for line/column spacing
Last modified: 2018-02-02 20:31:53 UTC
Bug 781479 added line/column spacing, primarly for accessibility purposes. Chances are that many people would prefer to have it, but don't do an exhaustive web search (let alone after every version update) to find this option. So I think this feature should be more discoverable. Approach 1: Add 2 horizontal sliders (or some other widget) to the "General" tab of profile prefs, under the font chooser button. Easy to implement, there is room for that, although we might not want to pollute that tab too much. Approach 2: Add 2 horizontal sliders (or some other widget) to the font picker dialog (below the preview text and the size slider, above the Cancel/Select buttons). Needs some work since GtkFontButton's implicitly invoked GtkFontChooserDialog needs to be replaced by a brand new explicit window embedding a GtkFontChooserWidget, the sliders, plus manually repeating the Cancel/Select buttons, all with the same paddings as the non-extendable GtkFontChooserDialog. Has the weird usability drawback that the preview text doesn't reflect the chosen character spacing (let alone line spacing). We could, however, eventually hide the built-in preview field and create our own one.
> Has the weird usability drawback that the preview text doesn't reflect the > chosen character spacing (let alone line spacing). We could, however, > eventually hide the built-in preview field and create our own one. Font and color pickers (additional popups of prefs) have this weird thing that they have OK/Cancel buttons to apply/revert the changes, whereas the rest of prefs apply immediately. Maybe we could modify font/color pickers to also apply immediately and then there'd be no need for a preview area. Alternatively, if we decide to keep the current behavior, a multiline preview with the desired font and line/column spacing could be implemented by embedding a VTE :) Anyway, this is for a subsequent change, let's stay at the two sliders for now.
Christian, let me know if you object, but I'm planning to implement the 2nd approach. Just its basic variant (two sliders; preview text not reflecting the spacing.) It's also a great warmup task for bug 722114 :)
I don't object to the UI proposal 2), but it's going to be tricky to implement, due to GtkFontButton having no accessor for the dialogue :-( An ugly hack that just might work would be to connect_after to 'clicked' on the font button, at which point the font dialogue should exist, in the list of toplevel window (gtk_window_list_toplevels()) and be transient-for the profile editor (so you can check out which GtkFontChooserDialog is the right one (there should be only one, but better to make sure)). Then when you have the dialogue, you can poke around it innards and add the sliders; and connect to response() to save the pref. Very very ugly, but the best I can think of that wouldn't need reimplementing all of gtkfontchooserbutton :-(
Good point. (I would have needed to actually start working on this to realize this problem.) Reimplementing the font chooser button sounds simpler and less hackish to me than your proposal. After all, it's just a button with two text labels and some vertical separator (and it's not even the end of the world if I don't exactly replicate its look). Or maybe this additional complication is a good reason to change our minds and revert to the much simpler 1st approach (sliders on the main tab)?
Well, not entirely trivial: $ wc -l gtkfontbutton.[ch] gtkfontchooserdialog.[ch] 1368 gtkfontbutton.c 114 gtkfontbutton.h 216 gtkfontchooserdialog.c 73 gtkfontchooserdialog.h 1771 insgesamt I guess the cleanest would be just to copypaste it into our sources with a s/GtkFontButton/TerminalFontButton/ etc, then just change the few bits directly to implement this. Or go with 1).
(In reply to Christian Persch from comment #3) > [...] at which point the font dialogue should exist, in the list > of toplevel window (gtk_window_list_toplevels()) and be transient-for the > profile editor (so you can check out which GtkFontChooserDialog is the right > one (there should be only one, but better to make sure)). ^^^^^^^^^^^^^^^^^^^^^^^^ This isn't correct, so I'd definitely need to locate the right one. There can be multiple profile pref dialogs open at the same time (one per profile) and one font chooser dialog for each. (In reply to Christian Persch from comment #5) > Well, not entirely trivial: > > $ wc -l gtkfontbutton.[ch] gtkfontchooserdialog.[ch] > 1368 gtkfontbutton.c > [...] > I guess the cleanest would be just to copypaste it into our sources with a > s/GtkFontButton/TerminalFontButton/ etc, then just change the few bits > directly to implement this. I'm not in favor of this copy-paste idea. As for the line count, it's not the button per se but the wiring between the button and the font chooser dialog which is cumbersome (and would also be cumbersome with our own button). > Or go with 1). I tend to go for this at this point :) and use the saved developer time on other bugs. Another advantage of this approach: scaling could be applied for the default system-wide font as well. With approach #2 you'd first have to check "Custom font" which potentially switches to a completely different font, and then try to find which one was the default if you liked it but just wanted to add some spacing. Instead of two sliders I'm also wondering about two number input fields aligned vertically with the initial terminal size's fields. I guess I should try and see which one looks better.
Approach 3: Instead of a button, just inline the font chooser widget. Perhaps moved to a new tab (General Command Font Colors...). I don't like it too much, just mentioned it for the record.
Created attachment 366055 [details] [review] v0 It's a good start. (Two horizontal sliders below the font chooser button.) (It's irritating how the terminal window keeps shrinking as you drag the slider, rather than retaining the logical size. I'll fix it in a separate patch.)
Created attachment 366059 [details] [review] Maintain logical window size Maintain logical window size... well, mostly. It works as long as you slowly move the slider. The window size begins to drift away though if you crazily drag the slider. There must be some asynchronity in the game, e.g. two events immediately following each other both working on the old window size, or something like that. I absolutely don't feel like tracking it down and fixing it, it might require understanding tons of GTK's internals really heavy refactorings for marginal benefit. We might want to switch to another UI presentation though, such as number boxes instead of the slider, to mitigate this problem. Also note that the warning of bug 791014 is present without this patch, but is gone with the patch. So I'm pretty sure that even though things aren't perfect yet, we should apply this patch (well, ideally should've been part of the initial cell-spacing work).
Comment on attachment 366059 [details] [review] Maintain logical window size Need the same for the vte test app, right?
(In reply to Egmont Koblinger from comment #9) > Created attachment 366059 [details] [review] [review] > It works as long as you slowly move the slider. The window size begins to > drift away though if you crazily drag the slider. > > There must be some asynchronity in the game, e.g. two events immediately > following each other both working on the old window size, or something like > that. I absolutely don't feel like tracking it down and fixing it, it might > require understanding tons of GTK's internals really heavy refactorings for > marginal benefit. Yes, not worth the work required. > We might want to switch to another UI presentation though, such as number > boxes instead of the slider, to mitigate this problem. Yes, a simple numeric entry may be better than trying to hit the desired target by dragging the slider. > Also note that the warning of bug 791014 is present without this patch, but > is gone with the patch. So I'm pretty sure that even though things aren't > perfect yet, we should apply this patch (well, ideally should've been part > of the initial cell-spacing work). Always, or just while changing the setting here? Because it would be beyond odd if that fixed it also elsewhere... E.g. I can get that warning every time I vertically enlarge the vte test app's window.
(In reply to Christian Persch from comment #10) > Need the same for the vte test app, right? The test app doesn't officially allow you to change the font-desc or line/char spacing runtime. You can tweak them (plus font-scale) with GTK inspector and then the window shrinks a bit, but honestly I just don't care. Zooming with hotkeys is handled there correctly by some other means.
(In reply to Christian Persch from comment #11) > > Also note that the warning of bug 791014 is present without this patch, but > > is gone with the patch. So I'm pretty sure that even though things aren't > > perfect yet, we should apply this patch (well, ideally should've been part > > of the initial cell-spacing work). > > Always, or just while changing the setting here? Because it would be beyond > odd if that fixed it also elsewhere... E.g. I can get that warning every > time I vertically enlarge the vte test app's window. Only while changing the line/char spacing, of course :)
> Yes, a simple numeric entry may be better than trying to hit the desired > target by dragging the slider. Let's hope we won't get bitten by bug 789591.
Comment on attachment 366059 [details] [review] Maintain logical window size "Maintain logical window size" submitted.
Created attachment 366091 [details] [review] v1 This now looks exactly as (and is aligned to) the initial terminal size line, that is: Initial term size: 80 -+ columns 24 -+ rows Reset Spacing: 1.00 -+ horizontally 1.00 -+ vertically Reset The advantage is that it's one line only, and IMO looks quite nice. The drawback is that there's no room for using the standard wording "letter spacing" and "character spacing", and even the shortest I could some up with that makes sense after the numbers, that is "horizontally" and "vertically" are too long and make the overall window noticeably wider (and probably it's even worse in many other languages).
(In reply to Egmont Koblinger from comment #16) > Initial term size: 80 -+ columns 24 -+ rows Reset > > Spacing: 1.00 -+ horizontally 1.00 -+ vertically Reset > > The advantage is that it's one line only, and IMO looks quite nice. It does :-) > The drawback is that there's no room for using the standard wording "letter > spacing" and "character spacing", and even the shortest I could some up with > that makes sense after the numbers, that is "horizontally" and "vertically" > are too long and make the overall window noticeably wider (and probably it's > even worse in many other languages). How about Inter-cell spacing: [ ]% width [ ]% height (and translating between the 1.0..2.0 pref value and 0..100% for the display/input)?
(In reply to Christian Persch from comment #17) > How about > > Inter-cell spacing: [ ]% width [ ]% height Will give a try. (Shouldn't it be "wide" and "high" then?) Is there a way to display the "%" sign nicely (next to the number, within the input field), or does it need to go outside to the label?
(In reply to Egmont Koblinger from comment #18) > Will give a try. (Shouldn't it be "wide" and "high" then?) Maybe? Not sure :-) > Is there a way to display the "%" sign nicely (next to the number, within > the input field), or does it need to go outside to the label? There's the "output" signal on GtkSpinButton for exactly this. But placing it outside may make it clearer that the input is in percent?
Created attachment 366121 [details] [review] v2 > > Will give a try. (Shouldn't it be "wide" and "high" then?) > Maybe? Not sure :-) Feels to me it's "40% wide" or "40% of width"... but I'd much rather trust your English than mine :) This version places the "%" sign outside, to the label. It's a bit weird to me that after all these debates whether the API should work with 0.0..1.0 or 1.0..2.0 numbers we finally settled with 1.0..2.0, probably mostly because of the well established wording of "single spacing" and "double spacing", and now on the UI it's whoops gone :-) I think I'd prefer to go back to 1.0..2.0 on the UI, if we could work out the labeling. But I'm okay with the current approach too.
(In reply to Egmont Koblinger from comment #20) > Created attachment 366121 [details] [review] [review] > v2 > > > > Will give a try. (Shouldn't it be "wide" and "high" then?) > > Maybe? Not sure :-) > > Feels to me it's "40% wide" or "40% of width"... but I'd much rather trust > your English than mine :) "of width" would be better, yes. But also longer, and you wanted short? ;-) > This version places the "%" sign outside, to the label. > > It's a bit weird to me that after all these debates whether the API should > work with 0.0..1.0 or 1.0..2.0 numbers we finally settled with 1.0..2.0, > probably mostly because of the well established wording of "single spacing" > and "double spacing", and now on the UI it's whoops gone :-) I think I'd > prefer to go back to 1.0..2.0 on the UI, if we could work out the labeling. Hmm that's true! How about something like this: Cell spacing: width × [1.0] height × [1.0] ?
> "of width" would be better, yes. But also longer, and you wanted short? ;-) Yup, and "wide" is even shorter :) > Hmm that's true! How about something like this: > > Cell spacing: width × [1.0] height × [1.0] Swap the parameters of the multiplication I guess: Cell spacing: [1.0] × width [1.0] × height otherwise it doesn't align with the initial size's 80 24 ?
Created attachment 366122 [details] [review] v3 So this is the same as v1 except for the labels.
Submitted.
We're gonna have some problems with the UI i18n... commit 3337e4b goes like +msgid "× width" +msgstr "anchura ×" Shall I just add a comment for translators, and the rest will be caught individually during testing? Or some more robust technical solution?
From past experience, translator comments only go so far, you have to continually monitor it's being followed :/ So I'd go for a technical solution; it's probably easy to just add two labels here, one with the '×' and the other with the translated string. Might *still* be good to add a translator comment telling how it'll be used.
Okay, I'll strip "× " from the translatable string. I'd still prepend it to the string runtime rather than having a new gtklabel, to get the right amount of space.
These are really weird strings, so some confusion (especially with the absence of translator comments) is to be expected. So what exactly is wrong with “anchura ×”? Not every language has the same syntax (word order) as English. I’m not saying it’s necessarily the right translation, just that it might be. :)
(In reply to Piotr Drąg from comment #28) > So what exactly is wrong with “anchura ×”? See the bottom row of https://bug722114.bugzilla-attachments.gnome.org/attachment.cgi?id=366304 for how this string appears on the UI. (Ignore the left-hand sidebar.)
Created attachment 366690 [details] [review] Ease translators work Something like this...
Comment on attachment 366690 [details] [review] Ease translators work Yes, that should do it. Thanks!
There's another potential problem with translations, see commit a5925fb: #: ../src/profile-preferences.ui.h:67 msgid "Rese_t" msgstr "A_lapállapot" #: ../src/profile-preferences.ui.h:75 msgid "R_eset" msgstr "_Visszaállítás" The two "Reset" labels are supposed to perfectly align (one for the initial terminal size, the other for the new cell spacing). (Both translations are absolutely fine here, the first one is a bit more like "Default", the second one is more like "Revert".) What could we do? Nag translators one by one, or just wait until someone reports them this inconsistency after noticing on the UI? Or go with the same mnemonic for the two fields, and let users press it once/twice to locate the desired button?
(In reply to Egmont Koblinger from comment #32) > There's another potential problem with translations, see commit a5925fb: > > #: ../src/profile-preferences.ui.h:67 > msgid "Rese_t" > msgstr "A_lapállapot" > > #: ../src/profile-preferences.ui.h:75 > msgid "R_eset" > msgstr "_Visszaállítás" > > The two "Reset" labels are supposed to perfectly align (one for the initial > terminal size, the other for the new cell spacing). > > (Both translations are absolutely fine here, the first one is a bit more > like "Default", the second one is more like "Revert".) > > What could we do? Nag translators one by one, or just wait until someone > reports them this inconsistency after noticing on the UI? Could either add a translator comment here plus file bugs are we notice it (or even add a test script for this, which compares the strings minus mnemonics). > Or go with the same mnemonic for the two fields, and let users press it > once/twice to locate the desired button? That would also be good enough, IMHO.
> > Or go with the same mnemonic for the two fields, and let users press it > > once/twice to locate the desired button? > > That would also be good enough, IMHO. Went for this one, it's just much simpler. These are also slightly destructive operations so it doesn't hurt if the user has to press yet another key to activate them :)