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 code quality of extension #23

Merged
merged 9 commits into from
Jan 10, 2025

Conversation

alanocallaghan
Copy link
Contributor

Resolve #22

  • Improve the size, spacing and alignment of the dialog box.
  • Clarify the purpose of buttons and options, and add tooltips. Also remove "threads" and make more generic
  • Clarify what "run" button does (and make it do something).
  • Consistently use resources
  • Add dialog title and set owner to the QuPath GUI
  • Remove deprecated API usage
  • Remove redundant addPreference method.
  • Make fields final if possible

@petebankhead
Copy link
Member

Thanks for doing this. Struggling to run it easily until #21 is merged - shall I do that now?

Building it & then installing the 'old' way with drag & drop, it's throwing an exception for me when I interact with the spinner in v0.6.0-rc3, and the dialog produced by the Java extension has weird resizing behavior.

I have mixed feelings on the use of resources. While we think of it as best practice now, I think it raises the barrier of entry considerably - and brings zero benefit for anyone at the minute, since nothing else in QuPath supports switching languages.

@petebankhead
Copy link
Member

I see November Me also had mixed feelings on the resources. Maybe a minimal extension example, and a best practices one? Or else just include in the javadocs that resources are optional?

@alanocallaghan
Copy link
Contributor Author

alanocallaghan commented Jan 10, 2025

Struggling to run it easily until #21 is merged - shall I do that now?

I did get it to run by using some intellij magic, but it didn't seem to actually modify anything on disk. Sure, they're independent.

Maybe a minimal extension example, and a best practices one? Or else just include in the javadocs that resources are optional?

I thought about 2 extensions but that seems like 2x the problems. Specifying in the javadoc makes sense.

the dialog produced by the Java extension has weird resizing behavior

Can you be more specific?

@petebankhead
Copy link
Member

the dialog produced by the Java extension has weird resizing behavior
Can you be more specific?

If we want to encourage best practice, this doesn't seem behavior to aspire to...

dialog

@alanocallaghan
Copy link
Contributor Author

Rather than implement good behavior, I simply removed the bad behaviour.

@petebankhead
Copy link
Member

I'd tend towards putting Run at the bottom, and ensuring that it fills the full width on resize (also a useful thing to learn).

Then there would be a little more horizontal spacing between the option and the spinner with the latter resizing to the remaining width on the row (the stage might be constrained to not get too big).

Alternatively, since spinners are somewhat obscure as controls go, it could be replaced with a TextField, and the Run button could result in a dialog being shown containing the text in the text field.

There might even be a combo box allowing to chose to make it an info, warning or alert. Then the demo starts to serve an important purpose, allowing people to generate hilarious dialogs to share.

fun

@petebankhead
Copy link
Member

I'm still seeing

Can't find resource for bundle java.util.PropertyResourceBundle, key integer.option.set-to (see full stack trace above, or use 'debug' log level)

any time I attempt to change the spinner (this time running from IntelliJ, after merging the latest commits from main)

@alanocallaghan
Copy link
Contributor Author

Yeah sorry about that

@alanocallaghan
Copy link
Contributor Author

alanocallaghan commented Jan 10, 2025

Alternatively, since spinners are somewhat obscure as controls go

I'd argue that they're not super obscure in the context of a qupath extension, and while having controls that are useful might be more fun I don't know if that's necessary to demonstrate the components of making an extension

Copy link
Member

@petebankhead petebankhead left a comment

Choose a reason for hiding this comment

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

I'm in favor of making a more fun and functional extension at some stage, since I think it's a training opportunity as well as a chance to be more motivating.

But I'm also in favor of merging this as it is, as I think it's a big improvement - so from my side it can be merged.

@alanocallaghan
Copy link
Contributor Author

Great. The motivation behind the integer spinner was to demo having a persistent preference that the user creates a UI for (rather than relying on propertysheet) so it'd be good to retain that.

@alanocallaghan alanocallaghan merged commit 4c148e0 into qupath:main Jan 10, 2025
1 check passed
@alanocallaghan alanocallaghan deleted the optimise-practices branch January 10, 2025 18:07
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.

Example extensions are hard to understand
2 participants