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.

Sign up for GitHub

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

Jump to bottom

Add Yes + Quit button to traverse confirmation dialogs #1607

Merged
merged 5 commits into from
Mar 8, 2023
Merged

Add Yes + Quit button to traverse confirmation dialogs #1607

merged 5 commits into from
Mar 8, 2023

Conversation

PawelLipski
Copy link
Collaborator

No description provided.

@PawelLipski PawelLipski added this to the v4.0.0 milestone Mar 7, 2023
@PawelLipski PawelLipski self-assigned this Mar 7, 2023
@PawelLipski PawelLipski requested a review from mkondratek March 7, 2023 16:27
@PawelLipski PawelLipski linked an issue Mar 7, 2023 that may be closed by this pull request
Dialogs opened by TraverseSyncToRemote should have Yes & quit button #1433
Closed
@PawelLipski PawelLipski modified the milestones: v4.0.0, v3.7.0 Mar 7, 2023
mkondratek
mkondratek reviewed Mar 7, 2023
View reviewed changes
config/checker/com.intellij.astub
@@ -353,8 +353,13 @@ class MessageDialogBuilder<T extends MessageDialogBuilder> {
}

class MessageUtil {
int showOkCancelDialog(@Tainted String title, @Tainted String message, @Tainted String okText, @Tainted String cancelText,
Icon icon, DoNotAskOption doNotAskOption, Project project);
int showOkCancelDialog(@Tainted String title, @Tainted String message, @Untainted String okText, @Untainted String cancelText,
Copy link
Collaborator

Choose a reason for hiding this comment

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

while we are here...

I am wondering 🤔 It must happen that sometimes we introduce some stubs and then, after a while modify the code so the stub becomes redundant 🗑️

  1. For now there is no mechanism to detect unused stubs, right?
  2. What if we bump IntelliJ version and the stubbed method/class no longer exists? Are there any errors in such case (I do not remember any)? Or is it ignored?
  3. (assuming 1. is true) Is it a problem the over time we will have more and more redundant stubs? E.g does it significantly affect the compilation/build time? If it is a problem then I might be worth to introduce some detection mechanism.

A bunch of questions 💐 I should have asked them years ago 😅

Copy link
Collaborator Author

@PawelLipski PawelLipski Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It must happen that sometimes we introduce some stubs and then, after a while modify the code so the stub becomes redundant 🗑️

:yes:, it does definitely... still, given the effort that's sometimes needed to figure out these annotations, I'd rather keep these stubs around (even if they might be of little value for the subsequent builds)

  1. For now there is no mechanism to detect unused stubs, right?

Nope, and I don't think there'll be any. It'd be hard to implement on our side, and I doubt Checker itself will ever support anything like that, given that stubs are kinda redundant by design (i.e. Checker has the entire JDK annotated, even though users will never actually use all of the JDK 😅)

  1. What if we bump IntelliJ version and the stubbed method/class no longer exists? Are there any errors in such case (I do not remember any)? Or is it ignored?

See https://checkerframework.org/manual/#stub-troubleshooting-type-checking-results... we use -AstubWarnIfNotFoundIgnoresClasses, so CF reports only missing methods/fields, but ignores missing classes, even if other classes from the same package are present

  1. (assuming 1. is true) Is it a problem the over time we will have more and more redundant stubs? E.g does it significantly affect the compilation/build time? If it is a problem then I might be worth to introduce some detection mechanism.

I doubt it really affects compilation times... given that Checker Framework itself ships with full annotated JDK!

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks! clear!

...n/java/com/virtuslab/gitmachete/frontend/actions/dialogs/TraverseStepConfirmationDialog.java Show resolved Hide resolved
@PawelLipski PawelLipski force-pushed the refactor/1433-extract-traverse-confirm-dialog branch from df57d99 to 0767f6a Compare March 7, 2023 18:08
@PawelLipski PawelLipski force-pushed the feature/1433-traverse-yes-and-quit branch 2 times, most recently from 56da138 to e285172 Compare March 7, 2023 20:18
@PawelLipski PawelLipski marked this pull request as ready for review March 7, 2023 20:19
Base automatically changed from refactor/1433-extract-traverse-confirm-dialog to develop March 7, 2023 20:52
@PawelLipski PawelLipski mentioned this pull request Mar 8, 2023
Merged
@PawelLipski PawelLipski force-pushed the feature/1433-traverse-yes-and-quit branch from c732926 to 00c4fe0 Compare March 8, 2023 15:21
mkondratek
mkondratek reviewed Mar 8, 2023
View reviewed changes
CONTRIBUTING.md
@@ -53,6 +53,10 @@ Use IntelliJ IDEA Community Edition/Ultimate.
Set `Class count before import with '*'` and `Names count to use static import with '*'` to a very high number (e.g. 500)
to avoid problems with CheckStyle when editing code, and remove exceptions like `javax.swing.*` from `Packages to Use Import with '*'`.

10. (optional) Go to `File > File Properties > Associate with File Type...`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

mkondratek
mkondratek approved these changes Mar 8, 2023
View reviewed changes
Copy link
Collaborator

@mkondratek mkondratek left a comment

Choose a reason for hiding this comment

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

tested. works fine 🚀

PawelLipski
PawelLipski commented Mar 8, 2023
View reviewed changes
CONTRIBUTING.md
@@ -283,10 +287,10 @@ Then, update `executors.docker_executor.docker[0].image` in [.circleci/config.ym

We follow [Semantic versioning](https://semver.org/) for the plugin releases:

* MAJOR version must be bumped for each plugin release that stops supporting any IDEA build (typically when `sinceBuild` is increased). <br/>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#1612 for further updates here

@PawelLipski PawelLipski force-pushed the feature/1433-traverse-yes-and-quit branch from df10fbe to dfff3e9 Compare March 8, 2023 16:15
@PawelLipski PawelLipski merged commit dfff3e9 into develop Mar 8, 2023
@PawelLipski PawelLipski deleted the feature/1433-traverse-yes-and-quit branch March 8, 2023 16:36
@github-pages github-pages bot temporarily deployed to github-pages March 8, 2023 19:34 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkondratek mkondratek mkondratek approved these changes

Assignees

@PawelLipski PawelLipski

Labels
traverse ux
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.

Dialogs opened by TraverseSyncToRemote should have Yes & quit button
2 participants
@PawelLipski @mkondratek