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

feat: Use dialog box for input #1217

Merged
merged 3 commits into from
Feb 2, 2022
Merged

feat: Use dialog box for input #1217

merged 3 commits into from
Feb 2, 2022

Conversation

quintesse
Copy link
Contributor

Detects when console input is not possible and will pop-up a dialog box instead.

This needs #1211 to be merged first.

@quintesse quintesse requested a review from maxandersen January 26, 2022 02:06
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #1217 (7d07618) into main (5de28c2) will decrease coverage by 0.44%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1217      +/-   ##
============================================
- Coverage     58.69%   58.25%   -0.45%     
  Complexity     1059     1059              
============================================
  Files            86       86              
  Lines          5707     5751      +44     
  Branches        962      974      +12     
============================================
  Hits           3350     3350              
- Misses         1862     1906      +44     
  Partials        495      495              
Flag Coverage Δ
Linux 57.01% <0.00%> (-0.44%) ⬇️
Windows 57.01% <0.00%> (-0.44%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/java/dev/jbang/cli/Edit.java 48.21% <0.00%> (+1.86%) ⬆️
...jbang/source/resolvers/RemoteResourceResolver.java 36.36% <0.00%> (-2.67%) ⬇️
src/main/java/dev/jbang/util/Util.java 56.36% <0.00%> (-4.88%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5de28c2...7d07618. Read the comment docs.

Copy link
Collaborator

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

lets make JBANG_GUI env that if false (and we for now assume its false by default) we work like before and if true we enable this behavior. Then we can release it and get some feedback before enabling it for every jbang install by switching to assume JBANG_GUI is true by default.

@quintesse
Copy link
Contributor Author

So how are we going to get feedback then if it's false by default?

@quintesse
Copy link
Contributor Author

You have to remember this will only kick in when somebody is using curl | bash and they're trying to run something that isn't trusted. If something would go wrong they can still use the trust command to pre-trust and then the pop-up wouldn't show.

Or we should start creating beta versions for Jbang that people who like to live on the edge can install :-)

@maxandersen
Copy link
Collaborator

So how are we going to get feedback then if it's false by default?

By asking them enable it :)

@maxandersen
Copy link
Collaborator

Btw. I didn't see it only happen using curl/pipe...are you sure ? I'll test again.

@quintesse
Copy link
Contributor Author

Btw. I didn't see it only happen using curl/pipe...are you sure ? I'll test again.

It should definitely only happen when the stdin is not a shell/terminal.

@quintesse
Copy link
Contributor Author

By asking them enable it :)

And how are you going to do that? Asking a bunch of people if they want to set JBANG_GUI=true in their environment? That's going to get as what? 3 people to test? :-)

Better to just ask them directly to download this version and test it for a bit. That way at least they won't forget to enable it.

But I think you're being a bit too careful :-)
If it works on my Windows and Linux machines and it works on your Mac we should be good.
If we notice that in some particular way it doesn't work we fix it and we release an update.
(Which is a good reason to also merge #1219 )

@maxandersen
Copy link
Collaborator

btw. how are you testing this ?I can for the love of me not activate it :)

@quintesse
Copy link
Contributor Author

Hehe, one of the ways is simulating a curl by simply using cat:

cat build/install/jbang/bin/jbang | bash -s - edit itests/helloworld.java

@quintesse
Copy link
Contributor Author

Sorry, you actually need to be in the bin directory for that to work, so:

cd build/install/jbang/bin
cat jbang | bash -x -s - edit ../../../itests/helloworld.java

@maxandersen
Copy link
Collaborator

okey so here is what I get
2022-02-02_01-22-54
:

notice the java duke icon..in the dialog AND in docker.

Can we get that to at leat be using jbang icon to not look too javaish :)

@maxandersen
Copy link
Collaborator

also needs rebase after #1211 got merged?

Jbang now detects if no console is available to ask for user feedback
and will try to pop-up a dialog box instead.

Fixes jbangdev#1216
@quintesse
Copy link
Contributor Author

quintesse commented Feb 2, 2022

notice the java duke icon..in the dialog AND in docker.

It's just a single method for opening a dialog with a single select.
It's not something that I designed, so I think it depends on your platform, because it doesn't look that on my systems.

image

@quintesse
Copy link
Contributor Author

Rebase done.

@quintesse
Copy link
Contributor Author

@maxandersen could you perhaps see if any of the other standard icon options give a better result?

JOptionPane.QUESTION_MESSAGE, null, options, defOpt);

It's easier if you test it manually instead of me making changes , committing and then asking you to test :-)

@quintesse
Copy link
Contributor Author

And it seems that the dock icon can only be changed with JDK 9+ : https://stackoverflow.com/a/56924202/2889926

@maxandersen
Copy link
Collaborator

For Java 8+ we would need to set -Xdock:icon=/path/myIcon.png

@maxandersen
Copy link
Collaborator

...I could fear that only work for some jdks though...

@quintesse
Copy link
Contributor Author

quintesse commented Feb 2, 2022

Yes, and it only works for MacOS and where would we get the icon from?

Edit: although we could probably add it to the jbang archive I guess.

Jbang will now try to set the application icon before showing any
dialogs. This is done using reflection because the necessary API was
only added to Java since version 9.
@quintesse
Copy link
Contributor Author

quintesse commented Feb 2, 2022

@maxandersen can you test if this new version works for you? There should now be Jbang icons both in the dialog and in the MacOS dock.

Edit: setting the "taskbar icon" doesn't seem to work on Linux and Windows:

[jbang] Unable to set application icon: java.lang.UnsupportedOperationException: The ICON_IMAGE feature is not supported on the current platform!

@maxandersen
Copy link
Collaborator

2022-02-02_17-57-47

works on java 11 at least

@quintesse quintesse requested a review from maxandersen February 2, 2022 17:04
Copy link
Collaborator

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

also tested on java 8 and here icon is great but task bar is duke but that I can live with :)

@maxandersen maxandersen merged commit dc3fcd9 into jbangdev:main Feb 2, 2022
@quintesse quintesse deleted the dialog branch February 2, 2022 19:52
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.

2 participants