Fix ButtonSet widget with icons resizing when UIScale is changed #3942

Gasparoken · 2023-06-29T15:24:01Z

No description provided.

Gasparoken · 2023-06-29T15:27:07Z

This PR will have two commits:

ButtonSet fix (ready for review)
MenuItems fix (in progress)

Gasparoken · 2023-06-29T16:56:35Z

Ready for review.

Gasparoken · 2023-06-29T22:30:17Z

I think some areas of the editor need resizing when UI scaling occurs, like the layer name cells on the timeline and perhaps the size of the color bar.

Gasparoken · 2023-06-30T16:20:55Z

Issues discovered in some windows like Export Sprite Sheet. It's not ready for review.

Gasparoken · 2023-07-12T18:56:06Z

Ready for review.
This PR has four commits to improve UI widget resizing via guiscale changes:

ButtonSet
MenuItems on the Menu
Layer name cell on the Timeline
Dialog windows (this fix saves unscaled size of the window, then load the scaled size to restore the window size with the adequate guiscale)

Possible improvement related to '4': save/load the center of the window frame instead the top-left pos.

Note: as per how '4' was implemented, stored preferences related to window sizes will result in unsuitable window sizes until the first dialog resize.
I think, if '4' is implemented, we will have to adapt the user preferences, regarding the window size values, to this new version.

dacap

Looks good so far! I've made some comments to review.

Just as a little tip if you enable the dev mode (ENABLE_DEVMODE=ON), you can use Ctrl+F1 to quickly switch/rotate between UI/Screen scaling values: https://github.com/aseprite/aseprite/blob/main/src/README.md#debugging-tricks

dacap · 2023-07-12T22:27:19Z

src/app/modules/gui.cpp

    Rect frame = get_config_rect(section, "WindowFrame", gfx::Rect());
    if (!frame.isEmpty()) {
+      // Re-scale the window size.
+      frame.w *= guiscale();
+      frame.h *= guiscale();
      limit_with_workarea(parentDisplay, frame);
      window->loadNativeFrame(frame);
    }


I think we can have two fields, one new "WindowFrameUnscaled" which is not scaled with guiscale() / is normalized, and use the previous "WindowFrame" when the new one is not available. Something like this:

Suggested change

Rect frame = get_config_rect(section, "WindowFrame", gfx::Rect());

if (!frame.isEmpty()) {

// Re-scale the window size.

frame.w *= guiscale();

frame.h *= guiscale();

limit_with_workarea(parentDisplay, frame);

window->loadNativeFrame(frame);

}

// First try to read the new "WindowFrameUnscaled" field (which is unscaled)

Rect frame = get_config_rect(section, "WindowFrameUnscaled", gfx::Rect());

if (!frame.isEmpty()) {

// Scale the window size with the UI scaling factor.

frame.w *= guiscale();

frame.h *= guiscale();

}

// If it's empty read the backward compatible "WindowFrame" field (which is

// already scaled with UI scaling)

else {

frame = get_config_rect(section, "WindowFrame", gfx::Rect());

}

if (!frame.isEmpty()) {

limit_with_workarea(parentDisplay, frame);

window->loadNativeFrame(frame);

}

dacap · 2023-07-12T22:28:26Z

src/app/modules/gui.cpp

+    // De-scale the window size to save it normalized.
+    rc.w /= guiscale();
+    rc.h /= guiscale();
    set_config_rect(section, "WindowFrame", rc);


And here we can save both fields (just to keep backward compatibility if the user open an older version of Aseprite):

Suggested change

// De-scale the window size to save it normalized.

rc.w /= guiscale();

rc.h /= guiscale();

set_config_rect(section, "WindowFrame", rc);

set_config_rect(section, "WindowFrame", rc);

// De-scale the window size to save it normalized.

rc.w /= guiscale();

rc.h /= guiscale();

set_config_rect(section, "WindowFrameUnscaled", rc);

dacap · 2023-07-12T22:29:52Z

src/app/modules/gui.cpp

    set_config_rect(section, "WindowFrame", rc);
    rc.offset(-mainNativeWindow->frame().origin());
    rc /= mainNativeWindow->scale();
  }
  else {
    del_config_value(section, "WindowFrame");
    rc = window->bounds();
+    // De-scale the window size to save it normalized.
+    rc.w /= guiscale();
+    rc.h /= guiscale();
  }

  set_config_rect(section, "WindowPos", rc);


Not sure what happens with this "WindowPos" field, it looks like this field now has changed too. Probably we need a "WindowPosUnscaled" too (used when multiple displays is disabled mainly).

Probably we need a "WindowPosUnscaled" too (used when multiple displays is disabled mainly).

Yes, I agree.

For the moment, WindowPos maintains the same position on the screen. This may be fine, but I feel like it's not correct in my opinion.
Example:

Guiscale 200%, go to File > Export > Export Sprite Sheet, suppose the dialog is centered on the screen. Close the dialog.

Change the guiscale to 100%, open again the Export Sprite Sheet dialog.

Result: The window will appear to be shifted to the top/left. When it would have been desirable to find it centered on the screen.

For this reason, I am going to propose to save the unscaled central position of the dialog, to be able to use it and re-scale it as desired guiscale (of course limiting the top/left corner of the window inside the editor).

src/ui/theme.cpp

dacap · 2023-07-12T22:39:49Z

src/ui/theme.cpp

+  int maxInt = std::numeric_limits<int>::max();
+  if (sz.w == maxInt || style->maxSize().w < maxInt)
+    sz.w = style->maxSize().w;
+  if (sz.h == maxInt || style->maxSize().h < maxInt)
+    sz.h = style->maxSize().h;


Probably something similar to the min case?

I also tried it: regression when guiscale is increased (for example 100% --> 300%). Some cases:

Playback control buttons on the timeline.

ContextBar > selection mode buttons with the Marquee tool active.

ContextBar > Radial/Linear buttons with thee Gradient tool active.

dacap · 2023-07-12T22:59:00Z

src/ui/widget.cpp

+  if (m_theme)
+    m_font.reset(m_theme->getWidgetFont(this));


This breaks the purpose of the m_font variable which is to cache the value of the widget font (to avoid asking the theme for the font again).

I would revert this change and give a try to this other (probable) fix in Widget::onBroadcastMouseMessage:

@@ -1691,6 +1690,9 @@ void Widget::onBroadcastMouseMessage(const gfx::Point& screenPos, void Widget::onInitTheme(InitThemeEvent& ev) { + // Reset cached font + m_font = nullptr; + // Create a copy of the children list and iterate it, just in case a // initTheme() modifies this list (e.g. this can happen in some // strange cases with viewports, where scrollbars are added/removed

dacap · 2023-07-12T23:03:38Z

As a side note @Gasparoken which I forgot to mention: there is no need to close a PR and open another one, you can just fix your branch/force push changes in your fix-uiscale branch and this PR is automatically updated.

Gasparoken · 2023-07-13T11:35:56Z

Thank you for the review, I'll take a look to this.

About commit '3' (Layer name cell on the Timeline), I'll change the approach.

As a side note @Gasparoken which I forgot to mention: there is no need to close a PR and open another one, you can just > fix your branch/force push changes in your fix-uiscale branch and this PR is automatically updated.

I used to do it this way, but in some cases, I want to preserve the history for some reason, for example: keep some comment from my reviewer with the well-referenced piece of code.

dacap · 2023-07-13T11:44:38Z

I want to preserve the history for some reason

Generally the history is best preserved if the PR is not replaced by a new one e.g. you can expand the resolved comments/review, see the history of the PR, if we replace it with a new PR the old one is a little lost in the history of the commit/new PR.

Another thing I'd like to fix in my workflow, is to rebase PRs properly with GitHub (this use case to rebase PRs is not well handled yet in GitHub with different emails, I'll try to fix this ASAP).

…change.

Gasparoken · 2023-07-13T23:11:07Z

The '5' commit is as a possible alternative way to manage the position/size of windows in a more intuitive way. What do you think @dacap?

Additional issue to solve: 1.3rc4: the size of the timeline dropdown settings menu is not managed correctly when guiscale is changed.

Gasparoken · 2023-07-14T20:16:21Z

Ready for review. Commit summary:

ButtonSet: fix updating bounds when guiscale is changed.
MenuItems on the Menu: fix updating bounds when guiscale is changed.
Layer name cell on the Timeline: keep the aspect size of the cell proportional to guiscale changes.
Dialog window save/load position/bounds: new approach to improve window bounds calculation.
Configuration Timeline window: fix updating bounds when guiscale is changed.

dacap · 2023-07-18T13:12:53Z

I would prefer to merge this PR without 6010a99 and b7d597a.

About 6010a99 I think I prefer to avoid modifying the native frame and just load/save the native frame as it is in "WindowFrame".

About b7d597a, I'm not sure what specific situation this patch fixes, but an important comment: redefining a non-virtual method (Widget::bounds()) in a derived class (ConfigureTimelinePopup::bounds()) will not behave as you expect (generally the original Widget::bounds() method will be called). And there are no plans to make bounds() member function virtual.

Gasparoken · 2023-07-18T14:04:19Z

About 6010a99 I think I prefer to avoid modifying the native frame and just load/save the native frame as it is in "WindowFrame".

This fix avoids involuntary window resizing and re-location. It's annoying if we change the UI scale frecuently
Example:
Initial condition: Screen scale = 100% , UI scale = 200%
Select > Color Range window:

Changing UI scale to : 100%
Select > Color Range window:

About b7d597a, I'm not sure what specific situation this patch fixes, but an important comment: redefining a non-virtual method (Widget::bounds()) in a derived class (ConfigureTimelinePopup::bounds()) will not behave as you expect (generally the original Widget::bounds() method will be called). And there are no plans to make bounds() member function virtual.

Example:
Initial condition: Screen scale = 100% , UI scale = 200%
Click on timeline configuration button:

Changing UI scale to : 100%
Click on timeline configuration button again:

This last issue can fix automatically re-starting Aseprite
Perhaps, this case can be solved by not saving the bounds of this specific window. I'll try this other approach.

On the other hand, the problem fixed in 6010a99 it's more damaging to the artist's workflow as a restart won't resolve saved custom window sizes (all windows with custom bounds should be resized once UI scaling is changed).

Alternatives:

Save different size/location for each available UIscale, or
When UI scale changes, reset all the custom window size/locations.

@dacap What do you think about this two alternatives? (I prefer "2")

dacap · 2023-07-18T14:22:28Z

Probably I'd prefer to merge the PR with the initial commits which already is a lot better than the current situation, and then discuss the other changes with more time and detail (I'm trying to prepare the next release probably for tomorrow).

Gasparoken · 2023-07-18T14:28:14Z

OK, I'll push-force a PR with the first three commits only. I'll create new issues for #3942 (comment)

Gasparoken · 2023-07-18T14:38:30Z

Tested and ready to merge.

Gasparoken self-assigned this Jun 29, 2023

Gasparoken force-pushed the fix-uiscale branch from 9bf459b to a8da408 Compare June 29, 2023 16:42

Gasparoken assigned dacap and unassigned Gasparoken Jun 29, 2023

Gasparoken marked this pull request as ready for review June 29, 2023 17:01

Gasparoken requested a review from dacap as a code owner June 29, 2023 17:01

This was linked to issues Jun 29, 2023

Menu items that don't respect UI scale #3843

Closed

Increasing UI Elements Scaling causes ui to become more squished #2571

Closed

Gasparoken assigned Gasparoken and unassigned dacap Jun 30, 2023

Gasparoken force-pushed the fix-uiscale branch from a8da408 to c33e474 Compare July 12, 2023 14:49

Gasparoken assigned dacap and unassigned Gasparoken Jul 12, 2023

dacap requested changes Jul 12, 2023

View reviewed changes

dacap assigned Gasparoken and unassigned dacap Jul 12, 2023

Gasparoken added 3 commits July 13, 2023 11:10

Fix ButtonSet widget with icons resizing when UIScale is changed

c188c81

Fix menu items that don't respect UI scale (fix aseprite#3843)

455947b

Fix layer name cell resizing in timeline during gui scaling or theme …

926ad68

…change.

Gasparoken force-pushed the fix-uiscale branch from 8a6f781 to 931689a Compare July 13, 2023 23:01

Gasparoken assigned dacap Jul 13, 2023

Gasparoken removed their assignment Jul 13, 2023

Gasparoken force-pushed the fix-uiscale branch 2 times, most recently from 351d7b3 to b7d597a Compare July 14, 2023 20:12

dacap assigned Gasparoken and unassigned dacap Jul 18, 2023

Gasparoken force-pushed the fix-uiscale branch from b7d597a to 926ad68 Compare July 18, 2023 14:31

Gasparoken assigned dacap and unassigned Gasparoken Jul 18, 2023

This was referenced Jul 18, 2023

Floating windows location/size should be repositioned/resized after UI scaling #3971

Open

Timeline settings window does not resize after UI scaling #3972

Open

dacap merged commit 98366d3 into aseprite:main Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ButtonSet widget with icons resizing when UIScale is changed #3942

Fix ButtonSet widget with icons resizing when UIScale is changed #3942

Gasparoken commented Jun 29, 2023

Gasparoken commented Jun 29, 2023

Gasparoken commented Jun 29, 2023

Gasparoken commented Jun 29, 2023

Gasparoken commented Jun 30, 2023

Gasparoken commented Jul 12, 2023 •

edited

Loading

dacap left a comment

dacap Jul 12, 2023

dacap Jul 12, 2023

dacap Jul 12, 2023

Gasparoken Jul 13, 2023 •

edited

Loading

dacap Jul 12, 2023

Gasparoken Jul 13, 2023

dacap Jul 12, 2023

dacap commented Jul 12, 2023

Gasparoken commented Jul 13, 2023

dacap commented Jul 13, 2023 •

edited

Loading

Gasparoken commented Jul 13, 2023

Gasparoken commented Jul 14, 2023

dacap commented Jul 18, 2023

Gasparoken commented Jul 18, 2023

dacap commented Jul 18, 2023

Gasparoken commented Jul 18, 2023

Gasparoken commented Jul 18, 2023

Fix ButtonSet widget with icons resizing when UIScale is changed #3942

Fix ButtonSet widget with icons resizing when UIScale is changed #3942

Conversation

Gasparoken commented Jun 29, 2023

Gasparoken commented Jun 29, 2023

Gasparoken commented Jun 29, 2023

Gasparoken commented Jun 29, 2023

Gasparoken commented Jun 30, 2023

Gasparoken commented Jul 12, 2023 • edited Loading

dacap left a comment

Choose a reason for hiding this comment

dacap Jul 12, 2023

Choose a reason for hiding this comment

dacap Jul 12, 2023

Choose a reason for hiding this comment

dacap Jul 12, 2023

Choose a reason for hiding this comment

Gasparoken Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

dacap Jul 12, 2023

Choose a reason for hiding this comment

Gasparoken Jul 13, 2023

Choose a reason for hiding this comment

dacap Jul 12, 2023

Choose a reason for hiding this comment

dacap commented Jul 12, 2023

Gasparoken commented Jul 13, 2023

dacap commented Jul 13, 2023 • edited Loading

Gasparoken commented Jul 13, 2023

Gasparoken commented Jul 14, 2023

dacap commented Jul 18, 2023

Gasparoken commented Jul 18, 2023

dacap commented Jul 18, 2023

Gasparoken commented Jul 18, 2023

Gasparoken commented Jul 18, 2023

Gasparoken commented Jul 12, 2023 •

edited

Loading

Gasparoken Jul 13, 2023 •

edited

Loading

dacap commented Jul 13, 2023 •

edited

Loading