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

Add early return to ToolBar::handleDPIChange in case it has no items #1376

Conversation

fedejeanne
Copy link
Contributor

If the toolbar has no items then the method still executes and sends 2 unnecessary OS calls:

  • OS.SendMessage(toolBar.handle, OS.TB_BUTTONSTRUCTSIZE, TBBUTTON.sizeof, 0);
  • OS.SendMessage(toolBar.handle, OS.TB_AUTOSIZE, 0, 0);

How to test

  • Modify the Snippet18 and remove all items in the toolbar
  • Run the modified Snippet18 with the vm arguments: -Dswt.autoScale.updateOnRuntime=true

Expected results

  • Same visual result as the code in master
  • The OS calls should not happen.

Copy link
Contributor

github-actions bot commented Jul 31, 2024

Test Results

   486 files  ±0     486 suites  ±0   7m 52s ⏱️ +21s
 4 151 tests ±0   4 143 ✅ ±0   8 💤 ±0  0 ❌ ±0 
16 358 runs  ±0  16 266 ✅ ±0  92 💤 ±0  0 ❌ ±0 

Results for commit 07ae825. ± Comparison against base commit aed1b96.

♻️ This comment has been updated with latest results.

@fedejeanne fedejeanne marked this pull request as ready for review July 31, 2024 08:19
@fedejeanne fedejeanne requested a review from niraj-modi as a code owner July 31, 2024 08:19
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Change looks sound to me. Would be good to also have your opinion, @akoch-yatta.

@akoch-yatta
Copy link
Contributor

akoch-yatta commented Aug 1, 2024

Change looks sound to me. Would be good to also have your opinion, @akoch-yatta.

I just had one scenario in my mind that could be a problem, I need to try that out.

@HeikoKlare I wanted to play around with scaling empty toolbar and extending that afterwards, but that works fine as well. So no objection from my side

No need to send OS events if there are no children to process in the
toolbar
@fedejeanne fedejeanne force-pushed the early_return_toolbar_handleDPIChange branch from e51f046 to 07ae825 Compare August 7, 2024 13:01
@fedejeanne fedejeanne merged commit 0af7bbc into eclipse-platform:master Aug 8, 2024
14 checks passed
@fedejeanne fedejeanne deleted the early_return_toolbar_handleDPIChange branch August 8, 2024 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants