Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
Merged #1738 into master. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#event-3918020249
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-716123908
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-716129879
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-716128828
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-716121373
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-706678337
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-706560587
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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] -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-706117255
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-706091629
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
Sorry, I was confused by the comments and didn't look at the code yet. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-359356434
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-359343024
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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: -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-357352193
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-357344169
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
Could it be set to `width/strlen(tmp_str)` from [here](https://github.com/geany/geany/blob/bc7e64f78f33ef867c9c1dfc2dc3ab76fd962ed1/src/sciwrappers.c#L108) to make it equal to one line number margin digit wide? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-357156244
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
> 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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-357154937
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-357150968
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
@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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-357150404
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1738#discussion_r161145541
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
@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. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-357149548
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1738#discussion_r161144957
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
So Scintilla resizes the symbols to match the margin width? (its not clear from Scintilla docs) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738#issuecomment-357148872
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1738#discussion_r161143710
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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` = ⅔
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1738#pullrequestreview-88383761
Re: [Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1738#pullrequestreview-88383628
[Github-comments] [geany/geany] Size symbols and fold margins proportional to line height (#1738)
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 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1738
