-
Notifications
You must be signed in to change notification settings - Fork 32
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
Feature/ansi console #40
Conversation
Handle ANSI escape sequences in console. Inspired from the ansi-econsole plugin
Could you please provide some example snippet showing ANSI input or even better a test that shows what is supposed to work now? May be also some screenshots? |
This plugin works for foreground/background colors, font style(italic/bold/underline/...). Others escape sequences are ignored (cursor movement, ...). Example snippet: |
An other functionality is that when a preference changes (background/foreground console color, interpret ANSI escape sequences, show escape sequences, ...) the console is update accordingly. |
get value directly from store. The getNewValue provide String instead of Boolean in case of Restore Defaults.
Thanks for your work. Since this seems to be heavily based on the Eclipse ANSI Console from @mihnita we have to/should reflect that when adding the copy-right headers. Because they have not been added before it probably has to be checked for each file who initially created it and all participants should be mentioned as contributors (since the git history is not retained). In general we need a CQ because the contribution has more than 1000 lines of code and probably also an IP-review? I also wonder the original code should be added with a first commit added as it is in the ANSI-console profile and then migrated in a second commit to the org.eclipse... namespace to better reflect who did what? And mentioned in Bug 112948 @mihnita started already some effort to contribute his ANSI Console to Eclipse: mihnita/ansi-econsole#70. Therefore I think it would be good if this work is coordinated to avoid duplication and that everybody gets the credit he deserves. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are already some preferences related to ANSI-support in the Console preference page. Do you plan to reconcile those.
Also, we need IP clearance for that code. Did you author all of it? If not, who are the other contributors? Are you willing to submit it under EPLv2?
org.eclipse.ui.console/src/org/eclipse/ui/console/AnsiConsolePageParticipant.java
Outdated
Show resolved
Hide resolved
org.eclipse.ui.console/src/org/eclipse/ui/console/IAnsiConsoleConstants.java
Outdated
Show resolved
Hide resolved
org.eclipse.ui.console/src/org/eclipse/ui/console/IAnsiConsoleConstants.java
Outdated
Show resolved
Hide resolved
org.eclipse.ui.console/src/org/eclipse/ui/internal/console/AnsiConsoleColorPalette.java
Show resolved
Hide resolved
org.eclipse.ui.console/src/org/eclipse/ui/internal/console/AnsiConsoleColorPalette.java
Show resolved
Hide resolved
@@ -0,0 +1,510 @@ | |||
package org.eclipse.ui.internal.console; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright header.
We're missing the PR that officializes this will to contribute ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to wait for @mihnita feedback.
org.eclipse.ui.console/src/org/eclipse/ui/internal/console/AnsiConsoleColorPalette.java
Show resolved
Hide resolved
org.eclipse.ui.console/src/org/eclipse/ui/console/IAnsiConsoleConstants.java
Outdated
Show resolved
Hide resolved
org.eclipse.ui.console/src/org/eclipse/ui/internal/console/AnsiConsoleColorPalette.java
Show resolved
Hide resolved
Sorry, I've only now noticed what is really going on. Pasting here the comment I've just added to https://bugs.eclipse.org/bugs/show_bug.cgi?id=112948
|
Note that my code had copyright notices with my name in
These seem to be removed in Yannick's code. |
He also seems prepared to publish his fork to Marketplace. Legal? Maybe. You guys should think if you want to get my code or his fork. In balance: someone who reliably maintained and improved this code, fixing issues, and in general being responsive? This is not some abandoned project that he revived. I am maintaining it (released a version just 2 weeks ago) And I was working towards integrate into Eclipse. Sorry for the tone, I am pretty angry right now. Thank you very much, |
@mihnita : I'm sorry, this was surely not intended, see #40 (comment) I would close this PR now. We will be happy to get your PR. |
Absolutely no problem. Thank you very much! My displeasure was not directed towards any of you ("the Eclipse guys"). My PR will come soon, I hope (see the entry about clearing it with my employer, for extra safety) Mihai |
Mihai, I first tried to contribute to your plugin to solve some performances issues and improve (in my opinion) the design and you rejected my PR. I decided to create a fork on my side (what you have seen in my repo) and to re-design other parts of the plugin. I shared with you all the issues I found to improve both your plugin and my fork. In a second time, I have seen the bug report Bug 112948 and since your last message in 2018 nothing had changed and I believed you abandonned the project to integrate it in Eclipse. After that I said that I would be interested to contribute and provided a preliminary work. Yannick |
@mihnita FWIW, I believe Eclipse Project doesn't need IP clearance from your employer to get the code in. The fact that it is EPLv2 (assuming license was decided correctly, ie all copyright owners have approved this license) is enough to get the code at Eclipse.org. So you could already proceed with building a pull request and we'll then get it processed by Eclipse IP Team. If this becomes too complicated, anyone is actually allowed to take care of the submission or merge. I don't want it to become an exclusive choice between @mihnita and @ydaveluy as contributor on Eclipse Platform. We expect both of you to contribute to the maintenance of ANSI console and to collaborate on that aspect. It's critical that everyone understands the code they submit in open-source is not only theirs any more and that it is very likely, and even desired, that some people fork/extend/modify it. The Eclipse Development Process describes how things must work in practice. Back to this particular case, the issue is that the discussion and progress that was taking place at mihnita/ansi-econsole#70 was not visible from this tracker nor former bugzilla, which interested parties like @ydaveluy are using as reference. So it's more a communication/visibility issue that triggers this parallel submission. |
Hi all,
Well, the part in the brackets is the tricky one :-) For those not familiar with copyright in the USA, your employer has copyright on anything you produce. All of the above is of of course my very rough summary of the proper legalese (that I don't really understand, as IANAL :-) But long story short: I wanted to be 100% that I don't mess up things.
Would reusing https://bugs.eclipse.org/bugs/show_bug.cgi?id=112948 be OK? Mihai |
And now, sorry, but some dirty laundry :-( I am sorry that I start this collaboration with the wrong foot. So I will try to explain a bit my reaction, before I bury it forever :-)
That's the legal part. Now the "ownership" and communication. I fully understand that under both Apache 2 and EPL 2 anyone is free to fork and do whatever they want with the code. But how it is done also matters. And it looks like the general consensus is that forking is bad:
All articles I've linked are very good reads. In general it looks like there should be good some reasons for a fork I don't think that was the case here. The pull request that I rejected was with explanations why, and with a request to separate the bug fix, the refactoring that improve performance (if any), and refactoring based on taste. And removed existing functionality. This was a PR out of nowhere, with no prior communication. On the communication side, indeed https://bugs.eclipse.org/bugs/show_bug.cgi?id=112948 does not reflect mihnita/ansi-econsole#70 But Yannick was in direct contact with me, and I was responsive, and I actually fixed most of the bugs:
His offer to contribute to Eclipse was June 20. So all he had to do before offering to fix Bug 112948 was to ask me "Hey, this bug is untouched for 3 years. If you are too busy, or you don't care, I intend to do it, if that's ok?" For example this was me asking the author of m2e-chromatic (forcing colors for Maven) if it is OK to implement that functionality in Ansi Console directly: sidespin/m2e-chromatic#3 And this is his offer to help: So, sorry, but I don't think I overreacted. With this out of the way, I hope we can focus on making this work, as professionally as possible. I know Eclipse is a very well established project, and probably has most things documented. Best regards, |
At this stage and given there have been good progress since the issue was 1st created, I think it's better to create a new fresgmissue on GitHub which would be more focused on this contribution.
OK, so please notify as soon as this tricky part is safe so we can move forward. |
I got the a copyright waiver from my employer. I've created the "Integrate in the ANSI Escapes in the Eclipse Console plugin #47" issue I've updated https://bugs.eclipse.org/bugs/show_bug.cgi?id=112948 pointing to it. I expect to prepare a PR over the week-end (4th of July, long week-end here :-) Regards, |
@mihnita Great news! Thanks! |
Hi,
This is a preliminary work.
The preference page does not support internationalization and should be improved (I am not an UI designer).
The AnsiConsolePageParticipant has an invalid reference. I don't know how to improve this part.
Performances are really good (better than the original ansi-econsole plugin).
To give you a comparison, to print 1_000_000 lines with 10/15 escapes sequences/line:
Please test it and give me your feedback.
Yannick