Opened 7 years ago

Last modified 2 years ago

#9189 new bug

[Terminal] handle custom font sizes in settings window.

Reported by: dknoto Owned by: jackburton
Priority: normal Milestone: R1
Component: Applications/Terminal Version: R1/Development
Keywords: Preferences, Font size Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Hi, I found and I fixed error of presentation the font size in dialog 'Settings' in Terminal application while manually setting this size in configuration file.

Before fix: http://dl.dropbox.com/u/92245884/Terminal-Font-Terminus-24p-Haiku.png After fix - below: http://dl.dropbox.com/u/92245884/Terminal-Font-After-Fix-Terminus-7p-Haiku.png After fix - inter: http://dl.dropbox.com/u/92245884/Terminal-Font-After-Fix-Terminus-13p-Haiku.png After fix - above: http://dl.dropbox.com/u/92245884/Terminal-Font-After-Fix-Terminus-20p-Haiku.png

Patch: http://dl.dropbox.com/u/92245884/Terminal.patch

Best regards

Attachments (6)

Terminal-Font-Terminus-24p-Haiku.png (155.9 KB) - added by dknoto 7 years ago.
Before fix
Terminal-Font-After-Fix-Terminus-7p-Haiku.png (59.6 KB) - added by dknoto 7 years ago.
After fix, font size below minimum of default list.
Terminal-Font-After-Fix-Terminus-13p-Haiku.png (71.3 KB) - added by dknoto 7 years ago.
After fix, font size inter values of default list.
Terminal-Font-After-Fix-Terminus-20p-Haiku.png (84.6 KB) - added by dknoto 7 years ago.
After fix, font size above maximum of default list.
Terminal.patch (1.9 KB) - added by dknoto 7 years ago.
Terminal-2.patch (2.6 KB) - added by dknoto 6 years ago.
This is second patch, more elegant but less efficient.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 7 years ago by umccullough

Please use Trac's file attachment features - links to external files can break in the future, making review of older tickets impossible.

Changed 7 years ago by dknoto

Before fix

Changed 7 years ago by dknoto

After fix, font size below minimum of default list.

Changed 7 years ago by dknoto

After fix, font size inter values of default list.

Changed 7 years ago by dknoto

After fix, font size above maximum of default list.

Changed 7 years ago by dknoto

Attachment: Terminal.patch added

comment:2 Changed 7 years ago by dknoto

Has a Patch: set

comment:3 Changed 6 years ago by zooey

I see possibilities for improving the patch:

  • Please don't vertically align variable declaration - this just makes it harder to insert/remove declarations at some later time (and it doesn't follow our styleguide).
  • With the static array, the code for inserting a menu item is required at three different places. I'd recommend to use some vector class instead (e.g. BList, BPrivate::Array or maybe even std::vector), to which the selected menu size could just be added up front and then the loop could just iterate over all the items and add them.

Considering the actual problem: I personally don't think using a menu for font size selection makes much sense in the first place - I'd prefer to be able to select any font size I wish.

comment:4 in reply to:  3 Changed 6 years ago by dknoto

Replying to zooey:

I see possibilities for improving the patch:

  • Please don't vertically align variable declaration - this just makes it harder to insert/remove declarations at some later time (and it doesn't follow our styleguide).

Of course, this is not problem.

  • With the static array, the code for inserting a menu item is required at three different places. I'd recommend to use some vector class instead (e.g. BList, BPrivate::Array or maybe even std::vector), to which the selected menu size could just be added up front and then the loop could just iterate over all the items and add them.

This is good idea I'm going to done it.

Considering the actual problem: I personally don't think using a menu for font size selection makes much sense in the first place - I'd prefer to be able to select any font size I wish.

OK, but this is more work than I have spare time for this at the moment.

Changed 6 years ago by dknoto

Attachment: Terminal-2.patch added

This is second patch, more elegant but less efficient.

comment:5 Changed 6 years ago by jscipione

I'm afraid that my recent Terminal changes will collide with this patch, sorry.

comment:6 Changed 6 years ago by dknoto

Don't worry, I'll try to quickly adjust.

comment:7 Changed 6 years ago by dknoto

Last changes in configuration of Terminal window are very confised :( You can't save changes in one move.

comment:8 in reply to:  7 Changed 6 years ago by jscipione

Replying to dknoto:

Last changes in configuration of Terminal window are very confised :( You can't save changes in one move.

Can you be more specific about what it is you are confused about. in hrev44975 I wrote:

The Font size menu becomes a submenu of Font so that you can set both the font and font size at once.

By selecting a font size as a submenu of the font menu, you select a font family, for example VL Gothic regular, and a font size, for example 14pt simultaneously. If you want to select a font family but keep the font size the same you can also do that by selecting just from the top level font menu. You are not able to change the font size without also selected a font family in the Settings window, however, you can do that in the Font size menu option in the menu bar of Terminal.

comment:9 Changed 6 years ago by dknoto

I found them, but it is bit confusing for me, in Polish "Czcionka" is not the same as "Czcionka i rozmiar", ("Font" != "Font & Size").

I prefer "Font Selector Dialog" and I'm slowly working on this.

comment:10 in reply to:  9 Changed 6 years ago by jscipione

Replying to dknoto:

I found them, but it is bit confusing for me, in Polish "Czcionka" is not the same as "Czcionka i rozmiar", ("Font" != "Font & Size").

In English there is enough ambiguity around what "Font" means that it works. According to Merriam Webster a font is "an assortment or set of type or characters all of one style and sometimes one size" so combining font family and font size together is defensible.

There should be 3 menu levels, first to pick the the font family (DejaVu Mono, VL Gothic, Courier New, etc.), then the font style (book, bold, italic), and then finally the font size.

I prefer "Font Selector Dialog" and I'm slowly working on this.

Yes, that is the real solution, anything less involves compromise.

comment:11 Changed 6 years ago by dknoto

OK. I understand your point of view, but for me 2 menu levels are better. I just need to adjust the patch to protect against evil the size of the configuration file.

comment:12 Changed 6 years ago by dknoto

Today I repeated the tests set the font size in the Terminal application. I have a few minor comments, which together undermine the meaning of this change: a) three-level access to the parameter, the font size, is cumbersome, b) the font size in a file larger than the upper limit, the algorithm breaks down, c) the number of requests _MakeFontSizeMenu method depends on the number of fonts in the system, for me it's 16 times more or 15 unnecessary calls.

Of course this is only my point of view.

comment:13 Changed 6 years ago by jscipione

Do you suggest we go back to a separate Font and Font Size controls, perhaps even allowing you to select an arbitrary font size?

comment:14 Changed 6 years ago by dknoto

No. I just wanted to show that the benefits are debatable. I suggest that while calmly wait a few days, I will present for discussion the class FontDialog / FontSelector. Then I only need one button that invokes a dialog to select font parameters.

comment:15 Changed 4 years ago by pulkomandy

Summary: Error of presentation the font size in the dialog 'Settings'[Terminal] handle custom font sizes in settings window.

comment:16 Changed 2 years ago by Premislaus

I think that is a proper ticket. Terminal dost not remember font size after closeing and starting again.

comment:17 Changed 2 years ago by humdinger

It does, if you set the size in the Settings... dialog. The things you change in the Settings menu are only used for the current session, unless you do "Save as defaults" there.

Note: See TracTickets for help on using tickets.