This makes those margin better adapt larger font sizes and zooms.
Fixes #1733. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1738
-- Commit Summary --
* Size symbols and fold margins proportional to line height
-- File Changes --
M src/editor.c (2) M src/sciwrappers.c (25)
-- Patch Links --
https://github.com/geany/geany/pull/1738.patch https://github.com/geany/geany/pull/1738.diff
b4n commented on this pull request.
/* symbol margin visibility */
void sci_set_symbol_margin(ScintillaObject *sci, gboolean set) { if (set) { - SSM(sci, SCI_SETMARGINWIDTHN, 1, 16); + gint width = margin_width_from_line_height(sci, 0.88, 16);
`0.88` come from `16/18`, where 18 was the line height of the pretty regular font size on my system, and 16 the previous width.
b4n commented on this pull request.
@@ -166,7 +185,9 @@ void sci_set_folding_margin_visible(ScintillaObject *sci, gboolean set)
{ if (set) { - SSM(sci, SCI_SETMARGINWIDTHN, 2, 12); + gint width = margin_width_from_line_height(sci, 0.66, 12);
Similarly, `12/18` = ⅔
codebrainz commented on this pull request.
/* symbol margin visibility */
void sci_set_symbol_margin(ScintillaObject *sci, gboolean set) { if (set) { - SSM(sci, SCI_SETMARGINWIDTHN, 1, 16); + gint width = margin_width_from_line_height(sci, 0.88, 16);
I believe modern compilers will optimize constant local variables out completely.
So Scintilla resizes the symbols to match the margin width? (its not clear from Scintilla docs)
elextr commented on this pull request.
/* symbol margin visibility */
void sci_set_symbol_margin(ScintillaObject *sci, gboolean set) { if (set) { - SSM(sci, SCI_SETMARGINWIDTHN, 1, 16); + gint width = margin_width_from_line_height(sci, 0.88, 16);
@b4n, well, 18 might be right on your monitor, but its probably 36 on @codebrainz 4k monitor.
@elextr yes. Downside is that the cross doesn't get thicker when it gets bigger, so branches look very thin at very big sizes. The arrow's better though, and even the cross is better thin than super small.
b4n commented on this pull request.
/* symbol margin visibility */
void sci_set_symbol_margin(ScintillaObject *sci, gboolean set) { if (set) { - SSM(sci, SCI_SETMARGINWIDTHN, 1, 16); + gint width = margin_width_from_line_height(sci, 0.88, 16);
@codebrainz made const.
@elextr ? that's the point, we currently hardcode 16px, which is very small if you use a big font/zoom level. For HiDPI the surface should be scaled in the best case (well, best case Scintilla would handle some of it itself I guess, but well), but otherwise this change just makes it better there too.
@b4n, ok, so bigger is better, so my only concern is the hard coded magic numbers that depend on monitor pixel density may not work on HIDPI monitors.
I'm actually tying to remove the hard-coded value and replace it by a ratio on the actual line height. The threshold value is here for small sizes to use all available line height space, e.g. not to use ⅔ of 3px or such.
Agree its better than fixed 16 pixels, thats silly. I'm just checking that it won't give a different set of silly results in some circumstances :)
BTW why did you scale it off the line height not the character width? Also does SCI_TEXTHEIGHT() include the configurable extradescent/ascent or not?
BTW why did you scale it off the line height not the character width?
Because Scintilla's filling the rectangle [margin width, line height] with the symbol, and most symbols we use are square, so it's the most efficient use of space.
Also does SCI_TEXTHEIGHT() include the configurable extradescent/ascent or not?
Not sure, but my guess would be it's accounting for the whole height the line uses on screen. But that's and interesting question.
Could it be set to `width/strlen(tmp_str)` from [here](https://github.com/geany/geany/blob/bc7e64f78f33ef867c9c1dfc2dc3ab76fd962ed1...) to make it equal to one line number margin digit wide?
It could, but what's your reasoning?
As said, Scintilla caps the symbol size to min(line height, margin width), so my reasoning is that the closest margin width is to line height, the bigger the symbol, yet leave some margin on top and bottom.
Ok, if Scintilla limits the symbol size so its never cut off then the worst it can do is appear overly large at times. And well, your magic 0.88 is no worse than 16 :grin:
Can we increase the margin width? Arguably, when someone zooms in source code text he probably also wants the line numbers and icons to be bigger.
@kugel- not quite sure what you mean, this sets the margin width to 0.88 times the line height already.
Sorry, I was confused by the comments and didn't look at the code yet.
This doesn't apply cleanly because of #2140. However, I'm not sure what to do about that, as I feel like this one is way better than just setting the value manually to something to workaround the issue.
However, maybe there's another reason to want to increase the margin width regardless of the font size? Even then, I'd suggest then it would be better to make the ratio configurable rather than an absolute value again; but that would be an incompatible change, so I wonder what you guys think? @elextr @codebrainz @eht16
Hmmm, re-thinking it, if the user changes the font size to be able to read it (bigger), or to fit more in (smaller) its reasonable to expect that the margin symbols will expand or contract along with the font size.
But IIUC the problem is that Scintilla modifies the height of symbols automatically as the line height changes, but not the margin width. In fact the margin width constrains the maximum symbol size, resulting in symbols not growing as the font size is increased, and having excess space in the margin when the font size is decreased.
As you note, changing a fixed margin width setting as you change font sizes is not very user friendly, but #2140 is better than the original unchangeable width.
So the basic idea is sensible, and I see it as an improvement on #2140.
So having the width factored by something that depends on the font size is fine. In most cases a fixed fraction is also probably fine, but reading the comments above it appears that the magic number is optimised for our conventional square margin symbols, but if plugins use other symbols with different aspect ratios that could be a reason for making the factor variable. And also some users might just like more space. So there is some purpose for making the factor a setting.
Whilst clearly both automatic margin scaling and fixed width cannot be active at once, in theory it would be possible to have both and let the user choose which, but I'm not sure if its worth it, to me the automatic approach is strictly an improvement on a fixed setting, which is of course an improvement on no setting.
QED [end thesis]
Hum, I just noticed that #2140 has been merged in the current release window, so we don't have a released version with the setting, which makes me less worried about backward compatibility. The question whether there are "legitimate" use cases where the proposed adaptive version is not enough and requires a setting still stands, but I'm less worried as well, as there would be an alternative proposal that could still be improved upon.
My 2 cents: I think the adaptive version here is better than the previously introduced setting. If at all, we could keep the setting from #2140 but unset by default and it would overwrite the adaptive with if set. Though I don't think it's necessary because I guess (but it's really just a guess) that this implementation will suffice for most users.
So to make it short: replace #2140 with this one.
Should we merge this for 1.37? The advantage is that #2140 has landed in this window, so reverting it now would impact fewer users. For the rest, see above.
I believe this is stable and safe, but feel free to disagree.
I did a last minute addition, that is properly recalculate margins sizes when changing the font. This can be left out if wanted, it's not critical for the feature, and we already have a bug there that line numbers margin wasn't resized eithe: try setting a font size super small (e.g. 4pt), restart Geany (or zoom in/out), and then set a larger font size (let's say 12pt): line numbers will be cropped. So I guess it's not something common enough that people notice.
line numbers will be cropped.
I thought Scintilla re-sized the numbers margins depending on the maximum number of digits?
But yeah its a different problems, not part of this.
I thought Scintilla re-sized the numbers margins depending on the maximum number of digits?
Scintilla doesn't size the margins itself, we have to tell it the pixel size. We calculate it, but we forgot to update that value when changing font.
But yeah its a different problems, not part of this.
It is related as not updating this fails to resize the fold margin size when changing font as well.
Let's get it in as it seems to work properly and because of the easier migration of #2140 within the 1.37 release. Things will get more complicated for users after 1.37.
Merged #1738 into master.
github-comments@lists.geany.org