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

Avoid print icons when running on Windows OS #14722

Merged
merged 1 commit into from
Mar 2, 2021
Merged

Avoid print icons when running on Windows OS #14722

merged 1 commit into from
Mar 2, 2021

Conversation

ejba
Copy link
Contributor

@ejba ejba commented Jan 31, 2021

Closes #14219

@ghost ghost added area/codestarts area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform labels Jan 31, 2021
@geoand geoand requested review from ia3andy and gastaldi February 1, 2021 16:43
@ejba ejba requested a review from gastaldi February 2, 2021 08:20
gastaldi
gastaldi approved these changes Feb 2, 2021
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll let @ia3andy have the final word ;)

Good job!

ia3andy
ia3andy requested changes Feb 2, 2021
View reviewed changes
Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

Well, I think I would prefer having only getIcon (no getAlternateIcon), and have the switch inside the getIcon so that anyone calling it won't have any OS problem.. wdyt?

@ia3andy
Copy link
Contributor

ia3andy commented Feb 2, 2021

We shouldn't forget that we also have wrong display for those: https://github.com/quarkusio/quarkus/blob/master/independent-projects/tools/message-writer/src/main/java/io/quarkus/devtools/messagewriter/MessageIcons.java

ia3andy
ia3andy approved these changes Feb 2, 2021
View reviewed changes
Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

Nice :)

@gastaldi
Copy link
Contributor

gastaldi commented Feb 2, 2021

We shouldn't forget that we also have wrong display for those: https://github.com/quarkusio/quarkus/blob/master/independent-projects/tools/message-writer/src/main/java/io/quarkus/devtools/messagewriter/MessageIcons.java

Ah nice catch. Perhaps those need to be refactored as methods? It would be nice if those constants were not public

@ghost ghost added the area/jbang Issues related to when using jbang.dev with Quarkus label Feb 2, 2021
@ia3andy
Copy link
Contributor

ia3andy commented Feb 2, 2021

@ejba could you give a screenshot of the output when creating a project on Windows?

@ejba
Copy link
Contributor Author

ejba commented Feb 2, 2021

@ia3andy I have to launch a VM for that, it would take time.

@ia3andy
Copy link
Contributor

ia3andy commented Feb 2, 2021 •
edited
Loading

@ia3andy I have to launch a VM for that, it would take time.

@ejba No worries, we may have them in the CI ;-)

@ejba
Copy link
Contributor Author

ejba commented Feb 2, 2021

It didn't work out. Should be use an asterisk instead?

@ejba ejba changed the title Print bullets instead in windows OS [WIP] Print bullets instead in windows OS Feb 2, 2021
@ejba ejba changed the title [WIP] Print bullets instead in windows OS WIP: Print bullets instead in windows OS Feb 2, 2021
@gastaldi
Copy link
Contributor

gastaldi commented Feb 2, 2021

It didn't work out. Should be use an asterisk instead?

Sure, that makes sense

@ejba
Copy link
Contributor Author

ejba commented Feb 2, 2021

How do we set up the encoding? Shouldn't it be specified on DefaultMessageWriter?

@ia3andy
Copy link
Contributor

ia3andy commented Feb 3, 2021

For MessageIcons, I think we could have text replacement for Windows OK_ICON = "[OK]"...

Also, I think changing MessageIcons class will break the API and can't be backported (cc @aloubyansky) but I am not sure?

For the codestart type icons, you may use - or >> or whatever similar..

@aloubyansky
Copy link
Member

Yes, it will break the backward compatibility. If it's still the desired change we can apply it at the time we extract the dev tools project into its own repo.

@ejba ejba changed the title WIP: Print bullets instead in windows OS Print alternative icons in windows OS Feb 3, 2021
ia3andy
ia3andy reviewed Feb 8, 2021
View reviewed changes
...-common/src/main/java/io/quarkus/devtools/commands/handlers/CreateProjectCommandHandler.java Outdated
@@ -108,7 +107,7 @@ public QuarkusCommandOutcome execute(QuarkusCommandInvocation invocation) throws
final CodestartProjectDefinition projectDefinition = catalog.createProject(input);
projectDefinition.generate(invocation.getQuarkusProject().getProjectDirPath());
invocation.log()
.info("\n-----------\n" + MessageIcons.NOOP_ICON + " "
.info("\n-----------\n" + " "
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this icon?, This was badly named but looked great:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a new icon named SUCCESS_ICON with the thumbs up and [SUCCESS] on Windows to use here and everywhere we used the NOOP to mean SUCCESS.

If there are some place [NOOP] is used a it should (I am not sure there is), we should also choose a new unix icon for it.

@ia3andy
Copy link
Contributor

ia3andy commented Feb 8, 2021

@gsmet @gastaldi @ejba did we decide to also change the icons on Unix systems? I really liked the way it looked before with thumbs up and all.

@ejba
Copy link
Contributor Author

ejba commented Feb 8, 2021

@ia3andy We don't have a final decision I think. I did a second commit to be easy to revert the changes. :)

@gsmet
Copy link
Member

gsmet commented Feb 8, 2021

@ia3andy esthetics is one thing (and it's very personal, I personally don't like emojis, too aggressive for the eye) but using emojis for status report is IMHO counter productive as harder to parse and making things different for Unix and Windows is also a problem as you can't have a tool properly parsing things for both without having to deal with both cases (and you have to think about it).

I'm -1 for using emojis on status reports.

@ejba
Copy link
Contributor Author

ejba commented Feb 8, 2021

@gsmet What should I do with the enum CodeStartType then?

@ia3andy
Copy link
Contributor

ia3andy commented Feb 8, 2021

@ejba wait a bit before we settle on this ;-)

@ia3andy
Copy link
Contributor

ia3andy commented Feb 8, 2021 •
edited
Loading

I think this should be decided on the quarkus-dev mailing list.

IMO the goal of those icons was purely esthetic and not meant to be parsed in the first place.., in the end maybe we should have both.

@ejba
Copy link
Contributor Author

ejba commented Feb 8, 2021 •
edited
Loading

Using both ways raise no harm I believe, and both of you get what you want.

@ia3andy
Copy link
Contributor

ia3andy commented Feb 8, 2021

So, if @gsmet is ok, we could:

  • For MessageIcons, use text AND icon when that compatible and only text when it's not
  • For codestarts types icons, keep it as already done in the PR, they are really not meant to be parsed

Then I think we should rename MessageIcons to MessageStatus, and add a new SUCCESS status to replace wrongly used NOOP.

@gsmet
Copy link
Member

gsmet commented Feb 8, 2021

Good for me!

@geoand
Copy link
Contributor

geoand commented Feb 17, 2021 •
edited
Loading

What's the status of this?

@ia3andy
Copy link
Contributor

ia3andy commented Feb 17, 2021

It's waiting for @ejba update.

@ejba ejba changed the title WIP: Print alternative icons in windows OS Avoid print icons when running on Windows OS Feb 21, 2021
@ejba
Copy link
Contributor Author

ejba commented Feb 24, 2021

@geoand done

@geoand
Copy link
Contributor

geoand commented Feb 24, 2021

@ia3andy mind doing a final check please and then merging if all is still OK?

Thanks

ia3andy
ia3andy requested changes Mar 1, 2021
View reviewed changes
...jects/tools/message-writer/src/main/java/io/quarkus/devtools/messagewriter/MessageIcons.java Outdated
private MessageIcons() {
OK_ICON("\u2705", "[SUCCESS]"),
NOK_ICON("\u274c", "[FAILURE]"),
NOOP_ICON("\uD83D\uDC4D", "[FAILURE]"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
NOOP_ICON("\uD83D\uDC4D", "[FAILURE]"),
NOOP_ICON("\uD83D\uDC4D", ""),

Copy link
Contributor

Choose a reason for hiding this comment

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

NOOP means no operation but doesn't mean failure at all. I would put an empty string her.

Copy link
Contributor

Choose a reason for hiding this comment

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

applying codestarts...
>> java
>> maven
>> quarkus
>> config-properties
>> dockerfiles
>> maven-wrapper
>> resteasy-example

-----------
[FAILURE] quarkus project has been successfully generated in:
--> target\extensions-test

:)

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW I don't think for this message we should use a NOOP, it should be a OK.

@ejba ejba requested a review from ia3andy March 1, 2021 08:53
ia3andy
ia3andy approved these changes Mar 1, 2021
View reviewed changes
Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

Thanks a lot @ejba, it took a few rounds but most important is this is very nice now..

@geoand geoand added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed area/jbang Issues related to when using jbang.dev with Quarkus area/platform Issues related to definition and interaction with Quarkus Platform labels Mar 1, 2021
@quarkus-bot quarkus-bot bot added the area/platform Issues related to definition and interaction with Quarkus Platform label Mar 2, 2021
@ejba
Copy link
Contributor Author

ejba commented Mar 2, 2021

I did a rebase to trigger CI again. It's supposed to pass in all tests, right?

@geoand
Copy link
Contributor

geoand commented Mar 2, 2021

The failure has nothing to do with the PR, we can merge

@geoand geoand merged commit 4056a98 into quarkusio:master Mar 2, 2021
@quarkus-bot quarkus-bot bot added this to the 1.13 - master milestone Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gastaldi gastaldi gastaldi approved these changes

@ia3andy ia3andy ia3andy approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/codestarts area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.13.0.CR1
Development

Successfully merging this pull request may close these issues.

Windows - ? instead of codestarts icons when generating project
6 participants
@ejba @ia3andy @gastaldi @aloubyansky @gsmet @geoand