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

Updating the control example by adding option to make Table and Tree Widgets editable #1239

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented May 17, 2024

Updating the example by adding option to make Table and Tree Widgets editable

This commit is part of testing the custom controls for multi-monitor support.

HOW TO TEST

  • Run the Runtime Workspace launch configuration
  • Open the SWT Controls view
  • Switch to the tab "Tree"/"Table"
  • Check the "Editable" option under "Other"
  • Edit the content of the tree/table

Contributes to #62 and #127

Copy link
Contributor

github-actions bot commented May 21, 2024

Test Results

   478 files  ±0     478 suites  ±0   6m 47s ⏱️ - 2m 41s
 4 137 tests ±0   4 129 ✅ ±0   8 💤 ±0  0 ❌ ±0 
16 341 runs  ±0  16 249 ✅ ±0  92 💤 ±0  0 ❌ ±0 

Results for commit 92ad33f. ± Comparison against base commit 80086c9.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

@ShahzaibIbrahim thank you for this contribution!

It looks good, just one detail: you don't need to add the controls to the SWT Custom Controls view since they are already in the SWT Controls view.

Please base your PR on top of master instead of akoch-master-33 to improve testability,

I changed the "How to test" in the description of this PR since manual testing is also testing

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

Last detail and then this PR is OK

fedejeanne
fedejeanne previously approved these changes May 21, 2024
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.

I have two remarks on this:

  1. Please format the code. There is currently a mixture of whitespace-based and tab-based indentation. Applying the auto-formatting to the changed/added code should result in proper indentation.
  2. Please adapt the commit message (in particular the title) to contain what the PR does and not what it might be used for. Currently the title is "Provide HiDPI Support for Multi-Monitor Environments for custom controls" but I do not see anything related to HiDPI support in the PR. Such information can be useful in the PR description, to make it easier for reviewers to understand use cases, and maybe also in the commit message, but the commit title (i.e., the first line of the commit message) should capture what the commit does and not what it might be used for.

Once these issues are addressed, I think we can also merge these improvement during freeze period as it only affects examples code that is not shipped with the SDK (just like we are allowed to still improve tests).

@ShahzaibIbrahim ShahzaibIbrahim changed the title Provide HiDPI Support for Multi-Monitor Environments for custom controls Updating the control example by adding option to make Table and Tree Widgets editable May 21, 2024
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.

Unfortunately, you have now applied some auto-formatting to the complete classes, so the commit is full of unrelated formatting changes. Please revert those formatting changes and only format the code that you have edited/added.

I addition, please ensure that the commit title fits into a single:
image

@fedejeanne fedejeanne dismissed their stale review May 24, 2024 07:39

New errors were introduced

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the akoch-master-33 branch 3 times, most recently from 759e5e3 to 9e985fd Compare May 24, 2024 09:53
@ShahzaibIbrahim
Copy link
Contributor Author

Unfortunately, you have now applied some auto-formatting to the complete classes, so the commit is full of unrelated formatting changes. Please revert those formatting changes and only format the code that you have edited/added.

@HeikoKlare I have applied formatting now only on my changes also changed the commit message

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.

I have some further comments on code complexity and code style.

This commit is part of testing the custom controls for multi-monitor support.
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.

Thank you for the contribution and for addressing my comments! The code looks good now. And the changes still work as expected.

@HeikoKlare
Copy link
Contributor

Merging, since freeze period check only fails because of M1 promotion calendar entry, but merging is actually allowed (and the change does not affect production code anyway).

@HeikoKlare HeikoKlare merged commit d95a444 into eclipse-platform:master Jul 5, 2024
13 of 14 checks passed
@ShahzaibIbrahim ShahzaibIbrahim deleted the akoch-master-33 branch July 10, 2024 10:20
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