Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove side panel min/max px limits #12787

Merged
merged 1 commit into from Nov 7, 2022

Conversation

dterrahe
Copy link
Member

@dterrahe dterrahe commented Nov 5, 2022

The side panel width was limited to an arbitrary 150-500 pixels, which does not make sense for hidpi or large fonts. Even with the lower boundary, the panel could be made too narrow for its contents forcing a horizontal scrollbar to appear and making some of the content invisible/not readily accessible, sometimes leading to confusion.

This PR removes those limits, instead preventing the user from making the panel wider than the window size and the (minimum) size of the top/bottom bars and other side panel allows.

The minimum width of the panels will be automatically adjusted to the needs of their content. Now that most widgets have been correctly ellipsized, this means uncompressible buttons. So for example the iop modules panel will be at least wide enough for 7 blending icons. A horizontal scrollbar will never appear.

fixes #12787

@TurboGit TurboGit added this to the 4.2 milestone Nov 6, 2022
@TurboGit TurboGit added the scope: UI user interface and interactions label Nov 6, 2022
@TurboGit TurboGit self-requested a review November 6, 2022 09:26
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing we loose here is the fix of broken/insane width values after a darktablrc corruption. All the key (<DT_UI_PANEL_*>_size) are not in darktableconfig and so don't have a min/max.

Would it have been possible to keep the entries but set min to 10 and max to 1000 for example?

@dterrahe
Copy link
Member Author

dterrahe commented Nov 6, 2022

All the key (<DT_UI_PANEL_*>_size) are not in darktableconfig

I could add them?

They don't need a "min", because if a value smaller than the size required by the widgets is set, it gets ignored anyway.

What would be an appropriate max? On a FH screen, 1000 would be good, on 4K, double that, etc. I was actually thinking of storing the current size as a percentage (of screen size or window size), rather than px. That would have the advantage that if I connected my 4K monitor to my laptop, the layout would stay the same. Or scale up the px with DT_PIXEL_APPLY_DPI. But that would change peoples current settings. If using percentage, we could treat anything above 150 (the current max) as a legacy setting and treat is as pixels, while saving new settings as percentage and treating anything below 100 as such.

@TurboGit
Copy link
Member

TurboGit commented Nov 6, 2022

Using percent of the current display size seems good to me. I would even suggest to use a new entry max_panel_height_percent to avoid any confusion.

EDIT: And set the max to 80% ?

@dterrahe
Copy link
Member Author

dterrahe commented Nov 6, 2022

max_panel_height_percent

You mean width? I haven't touched the height (of the bottom panel).

I guess what we are trying to avoid is that on restarting the side panel is so big that it is off the screen and it is hard to figure out what's going on. The limit isn't really going to be used when resizing, because that just looks at how much space is available (given the top/bottom and other side panel). So the only reason to make the limit configurable is if we expect it to misbehave in some way. In which case, it may be better not to have it at all? I think in the past the issue was more with the minimum size which could cause more serious problems (crashes, unable to grab or ever see the panel) but that should no longer be an issue anymore with this PR. If we use px sizes, then yes, starting on a FHD screen when the last size was set on a 4K one could be an issue.

What I'm worried about is that it might not be that easy to figure out screen size at startup, when the main window presumably hasn't been fully initialised yet. Maybe we start on another display (lower res) then the window eventually ends up and make it impossible to retain a larger panel size. Or if we look at the total area (across displays) we might still allow a panel that's larger than the window.

So I'm thinking that it might be better overall to set panel as % of window size (naturally capped at 100%, but we could also say 50% tbh that seems a pretty logical one; whatever per-panel limit we choose, if both of them use the max and the top/bottom are shown, the right one could still end up off-screen). Every time the top window gets resized we would resize the panels with it. That would take care of display (and resolution) changes too. But of course if would be a different behavior than people are use to...

@TurboGit
Copy link
Member

TurboGit commented Nov 6, 2022

I guess what we are trying to avoid is that on restarting the side panel is so big that it is off the screen and it is hard to figure out what's going on.

Yes, this or crash darktable. We had issues with some broken values making dt crash. The only option given to the user was : remove darktablerc. But you know that as IINM you've the one having worked on resetting the values when loading the resource file.

Now maybe there is no issue there, if the size if set to some insane large value and dt recover by using a proper size given the current display maybe we are safe and all this discussion is actually not needed.

@dterrahe
Copy link
Member Author

dterrahe commented Nov 6, 2022

IINM you've the one having worked on resetting the values when loading the resource file.

Not me. IIRC it was johnny-bit ? May be mistaken as well.

We had issues with some broken values making dt crash.

I think that would have been with 0 panel width, not too large.

Now maybe there is no issue there

With this PR, the worst case would probably be setting a wide panel on your 4k ultra wide monitor and then afterwards starting on an HD laptop display. The dt window would be forced to be (much) larger than the laptop screen and you'd have to resize the panel, resize the window, move the window and do it again a few times.

@TurboGit
Copy link
Member

TurboGit commented Nov 7, 2022

Ok, let's move forward I don't think we should over engineer this.

Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@TurboGit TurboGit merged commit 9353771 into darktable-org:master Nov 7, 2022
@dterrahe dterrahe deleted the no_panel_limits branch February 4, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants