ANSI Support: Cleanup context menu
Closed this issue · 22 comments
Move ANSI Support menus from the "global" console menu to the concrete console page context menu (same menu where default Copy/Paste entries are
- "Hijack" the standard Copy to do "Copy Text Without ANSI Escapes"
- Add "Copy Text With ANSI Escapes", after the default "Copy" entry
- Make sure "Enable / Disable ANSI Support" is in that menu and the checked/unchecked status is in sync with reality
Note that the status is updated by theplugin.xmlmechanisms (RegistryToggleState) - Unify the two "Preferences..." options ("ANSI Support Preferences..." and "Preferences...")
Please assign to me.
M.
Make sure "Enable / Disable ANSI Support" is in that menu and the checked/unchecked status is in sync with reality
Note that the status is updated by the plugin.xml mechanisms (RegistryToggleState)
The state is fixed by #64.
Regarding the context menu: I think this entry can stay in the default view options, as long as we have a global preference to disable/enable this. Moving the menu entry to the concrete page would only make sense if the support can be toggled independently for each console type. But that is surely a completely different task, if you would like to do this - please in a separated issue.
PR #67 does this:
- Removes the "ANSI Support Preferences..." option from the menu
- Modifies the existing "Preferences..." option (from the right-click menu) to also show the "ANSI Support Preferences..." page (in addition to the pages showing before, "Run/Debug" and "Console"
- Moves the "Copy Text With ANSI Escapes" and "Copy Text Without ANSI Escapes" options under the console page right-click menu
Looks reasonable?
Still to fine-tune in that area:
1. Copy options:
- The 2 copy options are still not next to the standard Copy
- I don't have a way to override the standard Copy behavior
- The 2 copy options are enabled even if there is nothing selected. The standard Copy option is disabled.
Personally I think the default behavior when I copy from a panel and with nothing selected should be to copy all.
I am permanently annoyed by having to Ctrl+A Ctrl+C (Select All - Copy), and Ctrl+C doing nothing when I forget to select. - Q: Do you think it would be acceptable to make the 2 copy handlers copy all if nothing is selected?
That would solve the "options not disabled when there is no text selected" part.
2. Enable / disable
- The enable / disable option is still under the "global" menu
I find that very undiscoverable. I've never seen that before, until I had to search where the ANSI methods went from the toolbar. - The enable / disable option has no visual indication if it is enabled or not. I look at it and can't tell.
- Q: Do you think it would acceptable to bring that icon back on the toolbar?
would acceptable to bring that icon back on the toolbar?
I don't think this is reasonable. Most of the people don't care/don't know anything about ANSI characters and if this feature is enabled and works as expected, 99.9% would never miss the option.
On the other side, all existing console pages would have an extra (useless) button which in most cases doesn't make any sense because there is no ANSI output at all.
So for sure, having button for every preference simplifies access to this concrete preference but this approach doesn't scale for such an extensible application like Eclipse.
would acceptable to bring that icon back on the toolbar?
I don't think this is reasonable.
ACK.
M.
This comment may be out of context, but one could perhaps have a preference for showing a toolbar button. E.g., we do that in Oomph where we don't want to 'pollute' the toolbar unconditionally:
Sorry if that doesn't make sense in this case...
Can you please do following:
- Squash changes from #67 in one commit
- Add commit from #70 to #67
- Move enable / disable option to the ANSI context menu and put separators above/below ANSI menu block
It is hard to review connected commits if they can't be cleanly rebased on top of each other.
- The enable / disable option is still under the "global" menu
With the #67 this doesn't make sense anymore. There is no need in a global option if that only shows in some consoles.
I'm not sure if this was intended or not, but the new context menus are only shown in the "default" process console (org.eclipse.debug.internal.ui.views.console.ProcessConsole and subclasses). Nothing is shown now in "other" console pages we have.
If this is intended (I personally would be fine with that), and we only want/need to support this console type, we should think about refactoring org.eclipse.ui.internal.console.ansi.participants.AnsiConsolePageParticipant contribution in plugin.xml and restrict this contribution to the appropriate console type / class.
- Move enable / disable option to the ANSI context menu and put separators above/below ANSI menu block
I am not sure what ANSI context menu
I will give some context, so that we can make the best decision.
Maybe the name of the plugin (when it was standalone) was somewhat confusing.
The name was "ANSI Escape in Console" (https://marketplace.eclipse.org/content/ansi-escape-console),
but at times / places I might have called it "ANSI Console".
Which is confusing, because it is not a Console.
It is "something that hooks on all the consoles build on top of a StyledText"
And implements LineStyleListener to interpret the ANSI escapes and produce styled output.
It currently works in at least in the "C/C++ Build Console", the "Maven Console", the "Host OSGiConsole", the console showing stdout / stderr for a lot running apps (Java, Perl, C/C++, Python, etc), and the Spring Tools Suite includes the plugin and (from all I know) implement some kind of REPL. I don't know if that is their own custom console or something build on top of an existing one.
TLDR: right now the enable / disable affects all consoles, and that is intentional.
So this was not intended:
I'm not sure if this was intended or not, but the new context menus are only shown in the "default" process console (org.eclipse.debug.internal.ui.views.console.ProcessConsole and subclasses). Nothing is shown now in "other" console pages we have.
And in all fairness I am not sure what can be done in a relatively short time (before 2022-09)
What do you think are the options?
Move the copy options back to the global console menu, where enable / disable is?
Or try to make them show in all the menu consoles? (and I might use some help with that :-)
And open an issue for the next release?
Not sure what the "proper" behavior should be.
Until now nobody complained about the icon in the global toolbar (for all consoles).
I'm not saying it was the right thing, or that it didn't (unintentionally) violate the Eclipse best practices.
Only that people seemed fine with the actions being global for all consoles.
For me it kind of made sense: I could see the icon, enabled or disabled, and the result in all consoles matched what the icon was telling me :-)
For me it kind of made sense: I could see the icon, enabled or disabled, and the result in all consoles matched what the icon was telling me :-)
Sure. You as the plugin author was looking for that because that is what your were interested most. But you are not the only Eclipse user we have :-)
Until now nobody complained about the icon in the global toolbar (for all consoles).
Same here. Because people installed ANSI Console bundle explicitly looked for ANSI support.
Now we are in the Eclipse platform that is used by millions of users in gazillions of different products.
The majority of "other" Eclipse users would never see any need in these buttons, and Console view is one of the most used in the IDE, with lot of 3rd party extensions. So polluting a well known UI part of the Eclipse with buttons for something that is rarely used makes no sense.
I don't know if you know the "EierlegendeWollmilchSau" experiment from @merks. It demonstrates very well how such "very important" global contributions to toolbars from N various projects might ruin user experience.
So what the platform provides now is "out of the box" basic ANSI escape support. Nice. If someone wants extra buttons related to "ANSI specific action XYZ", it could be easily added by a third party contribution, if the commands are defined, to any menu/toolbar in Eclipse. That can be done by your own original plugin, where you could still define extra keybindings / toolbar entries etc, re-using the platform code.
And in all fairness I am not sure what can be done in a relatively short time
We can leave "global" menu entries until we find a better solution. We can even hide them completely, and leave only commands that could be used by people who knows what they do, and your plugin would add them to toolbar.
I personally would not miss the ANSI menus, neither in context menu nor in the global menu/toolbar, but that's because I never had a need to read ANSI output.
So if you are OK,
- we can disable the ANSI menus for 4.25 and wait till you can hack "fully integrated" solution in the context menu.
- we can leave current state AS IS (btw, note already polluted console toolbar):
and may be additionally replace "ANSI Preferences" with "Preferences.." - Whatever else
I would choose 1).
Actually, looking on the #67 (where the ANSI specific actions are in the context of the "standard" runtime console) I would say, that could be a compromise, if you adds separators before/after and additionally moves the "enable/disable" to that menu from the global one.
I just wanted to note that Eclipse, at least for the global Toolbar, allows to specify actions that are initially hidden and then you can customize the perspective and enable them, so maybe that is something that could be used in case the button is useful for some.
Beside that, just some thoughts about "disabling" ANSI support (e.g. I never has to "enable" it on my bash or putty shell so most users would just assume it 'works') so I think there are two cases:
- I do not need ANSI because I know for sure that my console will never print any, so maybe it should be possible to do this even programmatically when contributing a console type
- I do actually need ANSI but for whatever reason still like to disable it (e.g. I like to see garbage in the console, or I disable ANSI output by my program by commandline options), but I don't think it needs to be very prominent or easy to switch on/off and might even be a setting on the Run Config!
@mihnita not sure if you have considered this already: Maybe it would even be possible to listen for ANSI escapes and only enable it on the first ANSI command? That way all plain text output would not be affected anyways?
@laeubi : I assume the main the reason for disabling is this:
and this:
The whole content added to every console goes over the regex. That plus extra overhead for creating and managing generated styles may cause performance issues.
Maybe it would even be possible to listen for ANSI escapes
In current implementation that would cause the code above to always run...
The whole content added to every console goes over the regex. That plus extra overhead for creating and managing generated styles may cause performance issues.
Disclaimer:
- I don't know much about ANSI escapes and how they work
- I don't know much about how console handles text-updates
but
Looking at the regexp, it seems ANSI escape sequence must begin with \u001b so I would assume one might first listen for text updates and if I found \u001b in the added text enable my "heavy" computations (maybe it would even be possible to completely handle the text as a stream of characters instead of regexp and make the computation itself much more lightweight).
Performance
Finding the positions is done in findPositions, called from calculateDocumentAnsiPositions, called from update(DocumentEvent event), which is part of the IPositionUpdater interface.
You are right, I can easily optimize it with a search for \u001b and return, and only use the regexp if I find something.
I can send a PR tonight.
In my testing I didn't find much of a bottleneck there (I go with "measure first, don't do premature optimization")
At some point I even tried to parse "by hand" (no regexp), but the performance was not much better.
The biggest performance bottleneck right now (measured) is the limited output buffer.
The positions found on document update are stored in the document itself (eventDocument.addPosition).
That is the standard Eclipse IDocument stuff.
When the console runs out of buffer, it starts deleting lines from the beginning.
And for each line deleted it looks for the positions contained in that line, and deletes them.
The positions are stored in an ArrayList, and the deletion happens from the beginning (oldest lines).
So if you have 1000 lines to delete, with 10 escapes per line (as it might be common in some logs, or reports),
the ArrayList.remove(0) is called 1000 x 10.
And every time the whole array is copied one entry over.
That is the biggest performance problem.
It is tracked here mihnita/ansi-econsole#66 and here #46
I think the best fix would be in the console code (there is a IOConsoleDocument or something like that)
The usage pattern of the console is consistent: add at the end, remove from the beginning (queue).
So the deletion is always from the beginning, and it is a compact block, no gaps.
It would be easy to optimize (delete from line x to y => find first position in x, find last position in y, remove range),
instead of remove one position at the time (see #46 (comment))
Easy experiment:
- write some code producing heavy ANSI escapes
- run and measure without the ANSI support installed (can be on 2022-09 M2, or 2022-06)
- install the ANSI public plugin, run and measure with the plugin disabled
- run and measure with the plugin enabled and the console set to "Limit console output" off (to avoid the problem described above)
The results are really really close, to the point that if you runs it several times the results without the plugin are sometimes higher than the results with the plugin.
I suppose that this happen because the escape sequences are usually very short (something like \u001b[1;31m or \u001b[m). So the regexp are very fast.
If one would try to abuse it (for example with a \u001b[....20K of digits and ; m) then I expect problems :-)
TLDR: I am interested to take a look at IOConsoleDocument (or whatever the name is), but I consider it outside the ANSI functionality, and no time for 2022-09.
I can submit a PR searching for \u001b and return for piece of mind.
But will probably not improve things much, if at all.
As a resolution for the current question ("cleanup context menu")
I hear you :-)
The main reason I've heard of for people disabling the plugin was for the performance problems explained before.
And that is usually per product :-(, not even console type.
So one does Java all day long, ANSI enabled all the time, except for that special case where we know it is slow.
Hence for some people having it in the toolbar was really handy.
I also had a test (in the plugin, removed here) that tested the console buffer size, and recommended a size increase (with a popup message box)
What about this:
- Move the copy options in the individual console context menu, with separators, as you show in your comment
- Remove the enable / disable option from the global menu / console menu / toolbar / everywhere.
- Maybe give a hotkey to enable/disable? (any suggestion?)
So if some people really need a quick way to disable / enable, they have it (until the next Eclipse release).
Of course, since this is a standard command, they can also go to Preferences ... Keys and add one themselves.
I would think for people who never used my plugin, things will "just work"
For those who used the plugin before, will see if they miss some of the functionality and complain.
And then we act on real user feedback, not on assumptions :-)
I didn't know about EierlegendeWollmilchSau experiment, thanks, interesting read.
What I usually follow in cases like this something Raymond Chen usually asks: "What if two programs did this?"
"I want my app to be first" / "always handle file type, and not allow users to change that" / "show a window that can't be covered"
In this case: what if every plugin decides that their options are so important that they deserve to be on the toolbar?
:-)
Or "what is several plugins want to add a menu option right after Copy" (or "replace the standard Copy" :-)
What about this:
- Move the copy options in the individual console context menu, with separators, as you show in your comment
yep
- Remove the enable / disable option from the global menu / console menu / toolbar / everywhere.
But leave in Preferences
- Maybe give a hotkey to enable/disable? (any suggestion?)
No.
So if some people really need a quick way to disable / enable, they have it (until the next Eclipse release).
Of course, since this is a standard command, they can also go to Preferences ... Keys and add one themselves.
Sounds overall like a good plan. Please adopt your PR. As for the keybinding, as you said: the command is there, user who need that can assign any keybinding they like. I personally always use "ctrl+3" to run any non standard command I need.
what if every plugin decides that their options are so important that they deserve to be on the toolbar?
Yes, that's the point.
As individual plugin authors we can do what is best for our plugins, because no one is forced to use our plugin and we are "free" in our decisions.
Whatever we do in platform must scale for 1000 plugins and work for most products out of the box, because they all are forced to use platform.
One better: I also remove the copy options, as you proposed, at least for now.
Right now they only show in one console (ProcessConsole), which is not good.
If I find an easy way (plugin.xml) to add it to all the consoles, with separators and all, fine, I'll create a PR.
If not, leave them out completely for 2022-09.
It is too late for big refactoring, with fancy menu customization in code.
Closed as "Fixed".
Move ANSI Support menus from the "global" console menu to the concrete console page context menu (same menu where default Copy/Paste entries are
Removed completely.
"Hijack" the standard Copy to do "Copy Text Without ANSI Escapes"
Not for now.
Add "Copy Text With ANSI Escapes", after the default "Copy" entry
Removed completely.
Make sure "Enable / Disable ANSI Support" is in that menu and the checked/unchecked status is in sync with reality
Removed completely.
Unify the two "Preferences..." options ("ANSI Support Preferences..." and "Preferences...")
Done.