GrandOrgue / grandorgue

GrandOrgue software
Other
148 stars 40 forks source link

Changed displaying an organ path in the Organs tab of the Settings dialog https://github.com/GrandOrgue/grandorgue/issues/1663 #1830

Closed oleg68 closed 3 months ago

oleg68 commented 3 months ago

This is the second PR related to #1663.

This PR switches displaying to the right part of path instead the left one.

Because wxListView does not have such capability, I replaced it with wxGrid and I rewrote most of methods manipulating this list.

But wxGrid can not store a custom item data. So I introduced theGOSettingsOrgan::m_OrganSlotPtrsByGridLine for this purpose.

oleg68 commented 3 months ago

When clicking randomly on different organs I noticed that the information below wasn't updated correctly, but was always delayed by one - ie the current info wasn't shown but the one I had clicked just before the current one.

Fixed

larspalo commented 3 months ago

@oleg68 Yes, now the data below is shown correctly when selecting organs. However, I really wonder if this PR brings anything really useful to the table? Perhaps you could shed some more light on what you really expect should be happening? This is a screenshot from just opening the File->Settings Organs tab:

wxGridVersionAtStart

Where as you can see the wxGrid is much too small for the available space and the text for Name and Path columns cropped and still starting at the left.

If I manually change size of the wxGrid columns I get this result:

wxGridVersionManualColChange

The manual resizing of the wxGrid is of course not remembered for the next opening of the dialog as it's created with absolute column widths every time. The pahts are aligned to the right - but that's not really useful nor aesthetic, in my opinion.

If you don't have any specific reasons for this behaviour - which I'd like to hear - I think the rework should have slightly different behaviour.

  1. The wxGrid (or like eariler wxListCtrl/wxListView if that would be better) should always fill the entire available horizontal space. This means that no absolute widths should be used for the columns.
  2. Only vertical scrolling allowed. Thus it shouln't be possible to horizontally overflow with the wxGrid.
  3. Any manual user changes of column widths should be remembered.
  4. Since width is limited to what's available - recalculations of other columns will be necessary when user changes the width.
  5. The paths should be left aligned if the whole string will fit.
  6. If the string for path won't be completely visible, the rightmost part of the string should be prioritized. This means re-calculation of the string to display depending on currently available width and text extent. The left part of the full original path string not currently fitting in the available width should be replaced with ... or something similar to indicate that not the whole string is visible.
oleg68 commented 3 months ago

@larspalo

  1. The wxGrid (or like eariler wxListCtrl/wxListView if that would be better) should always fill the entire available horizontal space. This means that no absolute widths should be used for the columns.
  2. Only vertical scrolling allowed. Thus it shouln't be possible to horizontally overflow with the wxGrid.

I disagree with 1 and 2, because it would make this dialog unusable on a small screen.

  1. Any manual user changes of column widths should be remembered.

Agree. It is coming soon in a separate PR.

  1. Since width is limited to what's available - recalculations of other columns will be necessary when user changes the width.

I wouldn't implement this because I prefer having a hosisontal scrollbar. The user is responsible of distributing the extra size across the columns unless he/she would like to use a scroll.

  1. The paths should be left aligned if the whole string will fit.
  2. If the string for path won't be completely visible, the rightmost part of the string should be prioritized. This means re-calculation of the string to display depending on currently available width and text extent. The left part of the full original path string not currently fitting in the available width should be replaced with ... or something similar to indicate that not the whole string is visible.

Neither wxListView nor wxGrid can do this out of box. But wxGrid allows to customize renderers, so I think it can be implemented. I'll do more research.

larspalo commented 3 months ago

I disagree with 1 and 2, because it would make this dialog unusable on a small screen.

Not really. With any reasonably modern size screen the available width would be greater than the hardcoded widths currently used. Even on my oldest netbook that only have 1024 px width this dialog isn't even close to unusable! It all depends on how one defines usable/not usable. The hardcoded pixel widths of the wxGrid/wxListView columns, for this kind of information display, are examples of poor UI design, in my opinion.

And of course, for any of the controls currently used, there are indeed some backend logic that needs to be coded in order to handle widths and strings gracefully. But as you said in #1832, this version, with some modifications, isn't worse than the current implementation so I'll eventually approve. However, the right aligned paths is not nice at all and certainly not what the request in #1663 actually meant (I outlined the first part of that request in point nr. 6 above). The secondary request was:

it would be helpful to resize the file to match resizing of the panel

which related to the string length adaptation - but I'd extend that to the (minimum) full width of the wxGrid/wxListView to follow along with changes of dialog/panel size.

As a compromise, if you really want the horizontal scrollbar possibility, in the longer run I'll still insist on filling the available width at the least so that the wxGrid cannot be smaller than that - which would give a much more pleasing layout - if someone wants to stretch it out further than that then let them do that and use the scrollbar. I understand that this also of course requires more coding as it won't happen automatically but would require catching the size events.

I do think that the right aligned paths are a worse implementation than the current, though.

oleg68 commented 3 months ago

I do think that the right aligned paths are a worse implementation than the current, though.

Agree. I'll try to find anothew way of displaying the right part of the path.

oleg68 commented 3 months ago

@larspalo I changed the implementation of the organs grid with the new custom renderer.

In the future, I'll do the same thing with the Organ selection dialog after merging this PR and #1832.

Remembering column sizes will be implemented after merging #1842.

oleg68 commented 3 months ago

@rousseldenis could you approve this PR?

oleg68 commented 3 months ago

@rousseldenis pourriez-vous approuver ce PR? Je ne peux pas creer un outre avant de le mergement.