[Terminal] handle custom font sizes in settings window.

[dknoto]

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

[umccullough]

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

[dknoto]

Before fix

[dknoto]

After fix, font size below minimum of default list.

[dknoto]

After fix, font size inter values of default list.

[dknoto]

After fix, font size above maximum of default list.

[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.

[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.

[dknoto]

This is second patch, more elegant but less efficient.

[jscipione]

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

[dknoto]

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

[dknoto]

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

[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.

[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.

[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.

[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.

[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.

[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?

[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.

[Premislaus]

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

[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.