Fix dialog w/scrollbar wrong size in a particular case (fix #5396) by Gasparoken · Pull Request #5481 · aseprite/aseprite

Gasparoken · 2025-10-23T12:28:55Z

aseprite-bot

clang-tidy made some suggestions

aseprite-bot · 2025-10-23T12:32:49Z

+          View* view = static_cast<View*>(dlg->grid.parent()->parent());
+          if (newBounds.size().w > workarea.w) {
+            newBounds.w = std::min(newBounds.size().w, workarea.w);
+            if (newBounds.size().h < workarea.h && view->horizontalBar())


warning: implicit conversion 'ScrollBar *' -> 'bool' [readability-implicit-bool-conversion]

Suggested change

if (newBounds.size().h < workarea.h && view->horizontalBar())

if (newBounds.size().h < workarea.h && (view->horizontalBar() != nullptr))

aseprite-bot · 2025-10-23T12:32:50Z

+          }
+          if (newBounds.size().h > workarea.h) {
+            newBounds.h = std::min(newBounds.size().h, workarea.h);
+            if (newBounds.size().w < workarea.w && view->verticalBar())


warning: implicit conversion 'ScrollBar *' -> 'bool' [readability-implicit-bool-conversion]

Suggested change

if (newBounds.size().w < workarea.w && view->verticalBar())

if (newBounds.size().w < workarea.w && (view->verticalBar() != nullptr))

aseprite-bot · 2025-10-23T12:32:50Z

 void Viewport::onSizeHint(SizeHintEvent& ev)
 {
  ev.setSizeHint(gfx::Size(1 + border().width(), 1 + border().height()));
+  for (auto child : children()) {


warning: 'auto child' can be declared as 'auto *child' [readability-qualified-auto]

Suggested change

for (auto child : children()) {

for (auto *child : children()) {

martincapello

Tested this and it seems to work. I'm just not sure how the expected UX should be, but this fixes the issue it claims to fix.

dacap

Some comments to work on, I'll be testing a little more tomorrow with multiple monitors.

dacap · 2025-10-27T23:07:54Z

  ev.setSizeHint(gfx::Size(1 + border().width(), 1 + border().height()));
+  gfx::Size sz = ev.sizeHint();
+  for (auto child : children()) {
+    sz += child->sizeHint();
+    ev.setSizeHint(sz);
+  }


I'm not quite sure if we should change the viewport size here (as it affects all viewports in the app), and have make this change explicit so the user must change the minimum size of the parent View to specify a minimum size (check that several elements in .xml widgets change the minwidth/height fields).

I'd prefer if we revert this change and in someway change the minimum size of the view explicitly (the view could be stored in a field of the Dialog class).

Just in case we need this change, we could do something like:

gfx::Size sz(1, 1); for (auto* child : children()) sz |= child->sizeHint(); ev.setSizeHint(sz);

But I'm not sure to apply this change to main directly (as it might break something else)

dacap · 2025-10-27T23:14:11Z

+        const bool allowVResize = bool(dlg->autofit & ui::TOP) ^ bool(dlg->autofit & ui::BOTTOM);
+        const bool allowHResize = bool(dlg->autofit & ui::LEFT) ^ bool(dlg->autofit & ui::RIGHT);


Is this right? This logic looks confusing, like it wants to say "allow vertical resize if top or bottom, but not both are enabled", but probably you want to specify it (a comment might help in these cases).

Should be this change to?

Suggested change

const bool allowVResize = bool(dlg->autofit & ui::TOP) ^ bool(dlg->autofit & ui::BOTTOM);

const bool allowHResize = bool(dlg->autofit & ui::LEFT) ^ bool(dlg->autofit & ui::RIGHT);

const bool allowVResize = bool(dlg->autofit & ui::TOP) | bool(dlg->autofit & ui::BOTTOM);

const bool allowHResize = bool(dlg->autofit & ui::LEFT) | bool(dlg->autofit & ui::RIGHT);

Maybe I'm confused in interpreting the API documentation that says: "...(eg. when autofitting with Align.LEFT | Align.TOP, the dialog bounds will shrink to the minimum size anchored at the top left of the dialog)". Therefore, I assume that if we anchor, for example, TOP and BOTTOM, the size would remain fixed given the anchoring concept. I'll need your confirmation this the behavior I'm describing is correct.
On the other hand, but following the logic of anchoring, how should a dialogue behave without Align?

It's as the API says, but never thought about the possibility of having TOP|BOTTOM or LEFT|RIGHT as anchors. The anchor should be one corner/edge (0, TOP|LEFT, TOP, TOP|RIGHT, LEFT, RIGHT, BOTTOM|LEFT, BOTTOM, BOTTOM|RIGHT), but probably we can support the case you say too.

Without autofit the default is TOP|LEFT (just changing the size without changing the origin).

dacap · 2025-10-27T23:16:15Z

+                            allowHResize ? dlg->window.sizeHint().w : oldBounds.w,
+                            allowVResize ? dlg->window.sizeHint().h : oldBounds.h);
+
+        if (auto* view = dynamic_cast<View*>(View::getView(&dlg->grid))) {


Probably it'd be better to have the view pointer directly in the Dialog class and doing something like:

Suggested change

if (auto* view = dynamic_cast<View*>(View::getView(&dlg->grid))) {

if (View* view = dlg->view) {

dacap · 2025-10-27T23:27:48Z

+          if (newBounds.size().w > workarea.w && allowHResize) {
+            newBounds.w = std::min(newBounds.size().w, workarea.w);
+            if (newBounds.size().h < workarea.h && view->horizontalBar() && allowVResize)
+              newBounds.h += view->horizontalBar()->getBarWidth();
+          }
+          if (newBounds.size().h > workarea.h && allowVResize) {
+            newBounds.h = std::min(newBounds.size().h, workarea.h);
+            if (newBounds.size().w < workarea.w && view->verticalBar() && allowHResize)
+              newBounds.w += view->verticalBar()->getBarWidth();
+          }
+          if ((dlg->autofit & ui::BOTTOM) && allowVResize)
+            newBounds.y = oldBounds.y2() - newBounds.h;
+          if ((dlg->autofit & ui::RIGHT) && allowHResize)
+            newBounds.x = oldBounds.x2() - newBounds.w;
+
+          // Trim of dialog areas outside the workarea
+          if (newBounds.x2() > workarea.x2())
+            newBounds.w -= (newBounds.x2() - workarea.x2());
+          if (newBounds.y2() > workarea.y2())
+            newBounds.h -= (newBounds.y2() - workarea.y2());
+          if (newBounds.x < workarea.x) {
+            newBounds.w = oldBounds.x2() - workarea.x;
+            newBounds.x = workarea.x;
+          }
+          if (newBounds.y < workarea.y) {
+            newBounds.h = oldBounds.y2() - workarea.y;
+            newBounds.y = workarea.y;
+          }

+          // Restore newBounds refered to the mainWindows
+          if (get_multiple_displays())
+            newBounds.offset(-mainWindowBounds.origin());
+        }
        dlg->setWindowBounds(newBounds);


I'll test this tomorrow, I've detected that with multiples monitors it doesn't work as expected. Wouldn't be possible to use fit_bounds() function in this case?

aseprite-bot · 2025-10-31T14:40:23Z

clang-tidy review says "All clean, LGTM! 👍"

aseprite-bot · 2025-10-31T14:42:19Z

clang-tidy review says "All clean, LGTM! 👍"

…5396) Prior to this fix, changing the visibility of a widget in an autoscrolling dialog would collapse the size of the dialog.

aseprite-bot · 2025-11-05T19:51:02Z

clang-tidy review says "All clean, LGTM! 👍"

dacap · 2025-11-10T21:57:49Z

I have to test this a little more this, but I'm not sure if we should modify the dialog size when it has a view/autoscrollbars, as the user might have changed the dialog size to something preferred, and modifying that size is just making things worse (changing the user preference / viewport size).

…5481) Prior to this fix, changing the visibility of a widget in an autoscrolling dialog would collapse the size of the dialog.

dacap · 2025-11-10T22:29:19Z

I've cherry-picked the commit in 71dd383 (as laf has conflicts), and I'll continue from there.

dacap · 2025-11-11T13:15:20Z

There are a couple of issues I'm trying to fix of this patch (now merged in main): 1) getCurrentWorkarea() doesn't return a rectangle on all paths, 2) a crash where dlg->view used but it's nullptr.

Fixed some regressions introduced in #5481 / 71dd383 * Replaced Dialog::getCurrentWorkarea() with ui::fit_bounds() (getCurrentWorkarea() wasn't returning a valid rectangle for all cases) * Fixed accessing a nullptr Dialog::view pointer * Use View::makeVisibleAllScrollableArea() instead of setMinSize() directly

Gasparoken requested a review from martincapello October 23, 2025 12:28

Gasparoken self-assigned this Oct 23, 2025

Gasparoken requested a review from dacap as a code owner October 23, 2025 12:28

aseprite-bot reviewed Oct 23, 2025

View reviewed changes

Gasparoken force-pushed the fix-dialog-size branch from dabfd00 to fd2865d Compare October 24, 2025 16:03

Gasparoken assigned martincapello and unassigned Gasparoken Oct 24, 2025

Gasparoken linked an issue Oct 24, 2025 that may be closed by this pull request

Modifying widget visibility on a dialog with autoscrolls shrink the dialog size #5396

Closed

martincapello approved these changes Oct 27, 2025

View reviewed changes

martincapello assigned Gasparoken and unassigned martincapello Oct 27, 2025

dacap mentioned this pull request Oct 27, 2025

BrushPopup and Dialog autoscrollbars fixes #5485

Merged

dacap assigned dacap and unassigned Gasparoken Oct 27, 2025

dacap requested changes Oct 27, 2025

View reviewed changes

dacap assigned Gasparoken and unassigned dacap Oct 27, 2025

Gasparoken force-pushed the fix-dialog-size branch 2 times, most recently from 23769c7 to ca090e2 Compare October 31, 2025 14:38

Gasparoken assigned dacap and Gasparoken and unassigned Gasparoken and dacap Nov 3, 2025

Fix dialog w/scrollbar wrong size in a particular case (fix aseprite#…

d8daa8b

…5396) Prior to this fix, changing the visibility of a widget in an autoscrolling dialog would collapse the size of the dialog.

Gasparoken force-pushed the fix-dialog-size branch from ca090e2 to d8daa8b Compare November 5, 2025 19:47

Gasparoken assigned dacap and unassigned Gasparoken Nov 5, 2025

dacap pushed a commit that referenced this pull request Nov 10, 2025

Fix dialog w/scrollbar wrong size in a particular case (fix #5396 / #…

71dd383

…5481) Prior to this fix, changing the visibility of a widget in an autoscrolling dialog would collapse the size of the dialog.

dacap closed this Nov 10, 2025

	if (newBounds.size().h < workarea.h && view->horizontalBar())
	if (newBounds.size().h < workarea.h && (view->horizontalBar() != nullptr))

	if (newBounds.size().w < workarea.w && view->verticalBar())
	if (newBounds.size().w < workarea.w && (view->verticalBar() != nullptr))

	for (auto child : children()) {
	for (auto *child : children()) {

		const bool allowVResize = bool(dlg->autofit & ui::TOP) ^ bool(dlg->autofit & ui::BOTTOM);
		const bool allowHResize = bool(dlg->autofit & ui::LEFT) ^ bool(dlg->autofit & ui::RIGHT);

	if (auto* view = dynamic_cast<View*>(View::getView(&dlg->grid))) {
	if (View* view = dlg->view) {

Uh oh!

Uh oh!

Conversation

Gasparoken commented Oct 23, 2025 • edited by martincapello Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aseprite-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martincapello left a comment

Choose a reason for hiding this comment

Uh oh!

dacap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aseprite-bot commented Oct 31, 2025

Uh oh!

aseprite-bot commented Oct 31, 2025

Uh oh!

aseprite-bot commented Nov 5, 2025

Uh oh!

dacap commented Nov 10, 2025

Uh oh!

dacap commented Nov 10, 2025

Uh oh!

dacap commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Gasparoken commented Oct 23, 2025 •

edited by martincapello

Loading