greatscottgadgets / packetry

A fast, intuitive USB 2.0 protocol analysis application for use with Cynthion.
BSD 3-Clause "New" or "Revised" License
154 stars 29 forks source link

nit: reduce `GtkColumnListView` spacing so tree branches visually connect #206

Open vk2seb opened 3 weeks ago

vk2seb commented 3 weeks ago

This is a very minor look-and-feel thing. Looking for opinions, happy to help out (e.g. PR) if you think it would be useful.

A normal packetry session might look like this: Screenshot_select-area_20241028221444

By switching to a bitmap monospace font (Terminus here) and reducing the vertical padding, the tree connections between rows meet up: Screenshot_select-area_20241028221329

There are still some horizontal tree misalignments above (which would need to be fixed). It would also make sense to only reduce padding in the central ListView and not everywhere in the whole application of course (this is happening as I'm applying the style to every element using the GTK debugger, however is just for the sake of example)

Does this sound like a reasonable change or are there some plan to move to different connector icons? (i.e graphical, non-ASCII)

martinling commented 3 weeks ago

Hi @schnommus,

Yes, it would be nice if we could get those to join up nicely.

In the longer term, I'd been thinking we'd substitute each of those text characters with graphical elements, so that we have more control over their appearance. But the rendering is always going to have to work on a per-row basis, so we'll need to eliminate the padding between rows either way.

The easiest way to do it is probably to eliminate all the vertical padding which gets between each rows' label containing the connecting lines, and then increase the font size of that label such that it ends up defining the overall height of the row, whilst leaving the rest of the content at the same size and spacing as it is now.

Even if we eventually implement a graphical version, I'd want to keep the text version behind the scenes. It's good for outputting the UI contents to a file for test purposes, and we might want a TUI frontend at some point in the future too.

martinling commented 3 weeks ago

By the way - I notice you moved the expander widgets to the left of the connecting lines too.

We used to have it that way, but we changed it to keep the expanders on the right, because in a complex capture the connections can get quite complicated, which would put the expanders a long way from the things they act on:

image

vk2seb commented 3 weeks ago

Hi @martinling,

Indeed as you suggested increasing the font size only in the connector cell makes this easier to achieve. Unfortunately I was not able to get the padding to exactly zero by modifying the various component properties, and had to apply a CSS rule to the label instead with a negative margin to get truly 'no' padding.

Now it's much closer to what I was imagining (also padded down the expanders so they are vertically centered):

image

Patch to get the above result is here (requires Terminus already installed): https://github.com/greatscottgadgets/packetry/compare/main...schnommus:packetry:seb/css-terminus

This is enough to make my eyes happy, but probably a fair bit more work would be needed to have the padding/sizing work correctly across all possible font/DPI settings.

Another thing I was experimenting with was replacing the with an , so the expander dropdowns smoothly open:

image

Doing this properly would require the connector to know whether an expander is open or closed (to choose a circle or an arc), but visually it feels a bit less cluttered. All a matter of opinion, just sharing some notes in case they are useful :smile:

martinling commented 3 weeks ago

Making the connectors vary with the expanded status is fairly straightforward.

Personally I do like the curved corners - I think I had a version which used them at some point. But if you're going to curve the top corners, please curve the bottom ones too! :grin:

See this commit on top of your branch: https://github.com/martinling/packetry/commit/04aefe47155e6dfe122ea07a3cf74088bca85eb4

vk2seb commented 2 weeks ago

Amazing thanks for the patch @martinling - it looks great! :) I've been using this quite a lot over the past days while debugging some host-side gateware.

The visual improvement seems very worthwhile. In any case it seems you've already done most of the work, feel free to close this if it gets merged!

martinling commented 2 weeks ago

Glad it's working well for you, but I think it's too dependent on theme/configuration to be a proper solution. This is what that branch looks like for me at the moment: image

I think the right approach has got to be to eliminate the vertical padding and then do it graphically. That way we can directly scale the symbols needed to fit the space allocated by GTK.

Oh and we're very excited about that host side gateware! Would be great to integrate that functionality directly into LUNA.

vk2seb commented 2 weeks ago

Oh yeah @martinling that definitely doesn't look right. The css should select the correct font everywhere without a local theme configuration, however if the terminus font is not installed or not found by the application then it would make sense that it looks like that. With the font installed it looks like this (of course the desired font should be packaged in the application instead for a real implementation):

Screenshot_select-area_20241104190503

But of course if we want it to work across any possible font at any possible size (which I'm sure you want) then this will be a little more work. I won't have time immediately to work on this but it will definitely stay on the back of my mind, so if nobody works on it in the meantime, I am happy to give it a shot in the next weeks

martinling commented 2 weeks ago

I do have the Terminus font installed (via the fonts-terminus package in Debian) and I can see it listed in LibreOffice etc, but I guess the CSS isn't causing it to be used.

Are you sure it's not just being used for you because of your own GTK theme/config here? You have all the rest of the text appearing in a monospace font too, which is not something I'd want us to make the default.

martinling commented 2 weeks ago

Ah, I needed to install xfonts-terminus to get the X11 version of the font.

However, I get weird results. There are intermittently some small gaps between some rows:

image

And sometimes when scrolling the display starts to oscillate rapidly between two nearby positions of the scrollbar.

vk2seb commented 2 weeks ago

Interesting, thanks for trying this out @martinling - that is indeed odd :( - I haven't noticed such artifacts here.

In an ideal world it should be possible to achieve this without touching CSS, although I was not able to get there by manually touching up each element in the ColumnListView using the GTK debugger. It's possible there was just one widget that I missed that had some padding which was causing things to not line up. It would be cleaner not to have to touch the CSS, it feels like a hack to use negative margin.

I guess an important side-topic here is how would one actually validate a change like this? Is it sufficient to just test a few different fonts and DPI settings and call it good, perhaps document at least which environment settings were tested and test it under every OS? I'm not sure if GTK looks exactly the same on every OS, and here where a single pixel difference is obvious, that quickly becomes important...

martinling commented 2 weeks ago

The gaps were due to the character which is missing from Terminus. If I substitute another for the end markers then there are no more gaps.

If I also change the CSS to use Terminus only for the conn_label class, the result looks pretty nice: image

However, I'm not keen on trying to bundle a specific font with the application, particularly given that on Linux this seems to specifically require the X11 version of that font - so the details are likely to vary with GTK's different rendering engines for each platform.

I think it would be better to move towards a graphical solution, which would eliminate the need for pixel-perfect font handling and would enable us to improve the appearance further.

vk2seb commented 2 weeks ago

I think it would be better to move towards a graphical solution, which would eliminate the need for pixel-perfect font handling and would enable us to improve the appearance further.

Thanks @martinling This is clear. In any case my reason for filing this ticket was more toward discussing the short-term solution (just playing with the fonts). But if that's off the table now, then feel free to close this bug and open a new one for a graphical solution.

If that's far out, perhaps intermediate-term, something like this could work:

This would allow anyone to achieve the above results and puts the onus of pixel perfection on the user, leaving the default look-and-feel as it is.

martinling commented 2 weeks ago

I suspect that just implementing the graphical solution is a better use of time than implementing a whole extra UI and settings system just for tweaking the text version.

But allowing users to customise the font used in the tree views is a reasonable feature request too.