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

Improve UI #240

Merged
merged 23 commits into from
Aug 9, 2022
Merged

Improve UI #240

merged 23 commits into from
Aug 9, 2022

Conversation

rock3r
Copy link
Contributor

@rock3r rock3r commented Aug 7, 2022

This PR implements an all-new UI for the plugin configuration and solves a number of issues.

The new UI is created using the Kotlin UI DSL; since our minimum supported version is 2020.3, I needed to implement the UI twice, both in the v1 and v2 DSL, for IJ < 21.3 and >= 21.3 respectively. The UIs are almost indistinguishable from each other from a user's perspective (but this required some hacking around on both DSLs).

I have added explanations (with links to detekt.dev) for the file-related settings, and made it possible to have multiple config files and plugins, with a decent UI. The paths are also treated system-independently and thus they are correctly relativized by the IDE in the settings xml file.

The settings file has changed in format, and there is a best-effort attempt at migrating from the old to the new settings format.

v1 DSL (legacy version for IJ < 2021.3) v2 DSL (for IJ >= 2021.3)
v1 v2 screenshot

Note: as mentioned, there's a number of hacks involved; the V1 ones are "forever" at this point, I hope the V2 ones will get better over time as we raise the minimum IDE version (21.3 is the next version that would make a difference, dropping the v1 DSL UI entirely; then 21.1 for some API changes)

Note: I have developed this under Windows. I haven't run this under Linux or macOS, although I don't expect this to cause issues.

Warning: I can't get the tests to run on my machine in any way, so I didn't touch those. They will likely need a pass to get them to work (although they do compile at least)

Warning: I have tested this new UI and it works fine, but I think it would be great if y'all folks gave it a test to see if I broke anything anywhere

Closes #231
Closes #135
Closes #117

@rock3r
Copy link
Contributor Author

rock3r commented Aug 8, 2022

@arturbosch can you approve the workflow to run?

@arturbosch
Copy link
Member

arturbosch commented Aug 8, 2022

@arturbosch can you approve the workflow to run?

Wow, screenshots look really good.
I approved the CI. Will review the PR later today and get back to your slack message 👍🏼

@rock3r
Copy link
Contributor Author

rock3r commented Aug 8, 2022

I have some minor tweaks to do to the v2 side of things after talking to folks on how to improve a couple of things, but should be pretty minor. I can get rid of a hack

@rock3r
Copy link
Contributor Author

rock3r commented Aug 8, 2022

I'll review what's wrong with the CI shortly

@rock3r
Copy link
Contributor Author

rock3r commented Aug 8, 2022

@arturbosch as expected tests are red, but unfortunately I can't see what failed because the test results output is not archived. Have you considered something like this?

@arturbosch
Copy link
Member

@arturbosch as expected tests are red, but unfortunately I can't see what failed because the test results output is not archived. Have you considered something like this?

I can confirm. Reverting the intellij plugin to 1.6.0 works.

@arturbosch
Copy link
Member

arturbosch commented Aug 8, 2022

Please take a look at the attached video. It seems that resizing is a bit off for the page now for the lists

Screencast from 08-08-2022 04:00:58 PM.webm
Screenshot from 2022-08-08 16-04-55

@arturbosch
Copy link
Member

This PR implements an all-new UI for the plugin configuration and solves a number of issues.

The new UI is created using the Kotlin UI DSL; since our minimum supported version is 2020.3, I needed to implement the UI twice, both in the v1 and v2 DSL, for IJ < 21.3 and >= 21.3 respectively. The UIs are almost indistinguishable from each other from a user's perspective (but this required some hacking around on both DSLs).

I have added explanations (with links to detekt.dev) for the file-related settings, and made it possible to have multiple config files and plugins, with a decent UI. The paths are also treated system-independently and thus they are correctly relativized by the IDE in the settings xml file.

The settings file has changed in format, and there is a best-effort attempt at migrating from the old to the new settings format.

v1 DSL (legacy version for IJ < 2021.3) v2 DSL (for IJ >= 2021.3)
v1 v2 screenshot

Note: as mentioned, there's a number of hacks involved; the V1 ones are "forever" at this point, I hope the V2 ones will get better over time as we raise the minimum IDE version (21.3 is the next version that would make a difference, dropping the v1 DSL UI entirely; then 21.1 for some API changes)

Note: I have developed this under Windows. I haven't run this under Linux or macOS, although I don't expect this to cause issues.

Warning: I can't get the tests to run on my machine in any way, so I didn't touch those. They will likely need a pass to get them to work (although they do compile at least)

Warning: I have tested this new UI and it works fine, but I think it would be great if y'all folks gave it a test to see if I broke anything anywhere

Works on Linux on IJ 2022.2 and AS Chipmunk. 👍🏼
However the apply button does not work for the checkboxes, only when configuration file is added.
Therefore the setting changes are not applied.

@rock3r
Copy link
Contributor Author

rock3r commented Aug 8, 2022

I can confirm. Reverting the intellij plugin to 1.6.0 works.

This didn't work for me, but downgrading Gradle to 7.4.2 did the trick :) Will be pushing soon.

EDIT: I had to downgrade both in the end :)

@rock3r
Copy link
Contributor Author

rock3r commented Aug 8, 2022

As for the apply/reset button: yeah there's something that's missing there. Let me look into it. I'll push the rest of the fixes in the meantime.

@rock3r
Copy link
Contributor Author

rock3r commented Aug 8, 2022

Please take a look at the attached video. It seems that resizing is a bit off for the page now for the lists

Screencast from 08-08-2022 04:00:58 PM.webm Screenshot from 2022-08-08 16-04-55

Yeah the minimum width is enforced by the UI DSL/list component itself, not something I have set. It looks like it's a bug with the DSL itself as this other page is also done with the DSL and exhibits the same behaviour:

image

The page you were comparing to is not using the DSL, so it doesn't have the same issue.

@arturbosch
Copy link
Member

arturbosch commented Aug 8, 2022

Please take a look at the attached video. It seems that resizing is a bit off for the page now for the lists
Screencast from 08-08-2022 04:00:58 PM.webm Screenshot from 2022-08-08 16-04-55

Yeah the minimum width is enforced by the UI DSL/list component itself, not something I have set. It looks like it's a bug with the DSL itself as this other page is also done with the DSL and exhibits the same behaviour:

image

The page you were comparing to is not using the DSL, so it doesn't have the same issue.

Can we somehow move the title just above the list like in this screenshot (Manage standalone scripts)?
I think then the alignment should be fixed. Or we wait for IJ to fix it ^^

Screenshot from 2022-08-08 16-54-43

@rock3r
Copy link
Contributor Author

rock3r commented Aug 8, 2022

I asked my colleague who works on the DSL if there's a fix for this — you can see the Swagger settings have the same issue without having a label on the side :)

@rock3r
Copy link
Contributor Author

rock3r commented Aug 8, 2022

FYI I tested without a label on the side and it makes no difference, it's definitely a DSL bug.

@arturbosch
Copy link
Member

As for the apply/reset button: yeah there's something that's missing there. Let me look into it. I'll push the rest of the fixes in the meantime.

👍🏼 ping me to test the apply changes and it's ready to merge (release)

@rock3r
Copy link
Contributor Author

rock3r commented Aug 9, 2022

I think I fixed the apply/reset issues. I've tested on 22.1, need to test on 20.3 too but it should be ok.

@arturbosch
Copy link
Member

I think I fixed the apply/reset issues. I've tested on 22.1, need to test on 20.3 too but it should be ok.

works now 👍🏼

Thanks for the awesome work! Please see my last comment and we are rdy to merge.

@rock3r
Copy link
Contributor Author

rock3r commented Aug 9, 2022

I did test on 20.3 as part of the last few bugfixes. I'm pretty happy with how this turned out :)

Will get the last thing done as soon as I have 5 min later today

@rock3r
Copy link
Contributor Author

rock3r commented Aug 9, 2022

All done :)

@rock3r
Copy link
Contributor Author

rock3r commented Aug 9, 2022

:partyparrot:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants