Add Basic Support for Named Command Handlers #1317
Conversation
|
I fetched this in my env, but any action in a plain text editor gives me stack overflows: |
|
@pcdavid I can't reproduce the Overflow in the normal text editor, but I found a way by opening the new dialog. |
|
No more stack overflow with the new version. I checked, I don't have this when using unpatched versions of the platform plug-ins. |
|
@pcdavid thanks for checking out, I don't know how I missed that. |
|
Thanks! This seems to work much better. I've just launched a subset of our (Sirius) tests suite with that, and most of them are OK (13 failures out of 3095 tests). It looks like some of the 13 failures could be related. |
|
@pcdavid I added a DisplayAsyncExec around the update, this should solve the issue. |
|
Thanks, I'll test this version later. |
I did not want to say that this is wrong, that I don't know either. But yes, it should work afterwards. |
I understand. Sorry if my wording sounded harsh, that was not the intent. |
No offense taken. I just hope that we can get this fixed while still supporting E4 Handlers. @merks can you take a look at the version number complains from Jenkins? I don't understand what is needed there as it looks like Jenkins wants an increase to the version that is already set. |
|
FYI, I don't see the "Invalid thread access" anymore with the latest version of the PR (e5ee308). |
|
I suppose there is no chance to get this in 2023-12 even if the code itself is reviewed and accepted soon? |
That's right. The regression is not new and the issue is not critical enough to fix it in a release candidate. |
N1k145
force-pushed
the
issue1314
branch
2 times, most recently
from
84b06e7 to
6985a2a
Compare
|
What's the state of this change. Master is open again. Can this be merged? |
|
@BeckerWdf From my side it should work, but somebody should look at the code and maybe test again. |
|
@pcdavid or @mickaelistria: Do you want to test again? |
I ran some tests (both basic manual tests and a subset of our test suite) and it seems OK. |
mickaelistria
force-pushed
the
issue1314
branch
from
6985a2a to
4637043
Compare
bundles/org.eclipse.core.commands/src/org/eclipse/core/commands/INamedHandler.java
Outdated
Show resolved
Hide resolved
|
Can we merge this @mickaelistria ? |
| * | ||
| * @return name of the Handler | ||
| */ | ||
| String getHandlerName(); |
There was a problem hiding this comment.
Sorry if I already asked this, but was it considered to add this as a default method in IHandler directly?
There was a problem hiding this comment.
When I implemented this my intention was to not add it to the IHandler interface directly as this is a somewhat niche use case and not supported everywhere where the IHandler is used.
What I think is open for discussion is that if this feature is something that should be generally supported, then also a proper E4 support is needed with an annotation (Named?) that provides this name.
But this should probably be handled in a separate PR.
There was a problem hiding this comment.
But this should probably be handled in a separate PR.
As this is API, we unfortunately won't be able to roll back if we add it.
As I'm looking more to #1314, I'm wondering why do we need labels on handlers here. Don't we already have a name on the command? Or is it that some operations have handlers without commands? (sorry for joining the discussion late and probably bugging with questions that were already considered)
There was a problem hiding this comment.
The handler can override the label from the command in some special cases, only in e3.
In E4 this is not possible, and we switched the Undo Redo handler to an E4 based handler bridge. (iirc)
There are a few cases where this is used and one case is the undo/redo. So the label basically says "Undo Typing" and not only "Undo" which is the name of the command.
There was a problem hiding this comment.
OK, got it. And indeed, if we want some dynamic label, only the handler can provide it.
However, I don't think it's so much of a "niche use-case" or at least that it's a use-case that we want to keep as "niche". I find it interesting that handlers enrich the action label to make it more precise. It's a good practice that should be kind of recommended.
So I think it's better to make it a new default method to encourage and make it easier for implementors in enriching the label.
There was a problem hiding this comment.
Or should this stay in a separate interface as it is optional?
default methods are optional to implement as well.
We don't strongly care about implementation here, we care more about APIs (which we can't easily change later).
I would still recommend that you make it a new default method in IHandler if possible, because it actually seems to provide more value than the current extension interface.
While you do that, I think it'd be better to remame the method to getHandlerLabel() because the name can be interpreted as an "id" which is not the case here. (Thinking more about it, it could be just getLabel() but there would be too high risks of conflicts so let's stick to getHandlerLabel(), and this makes me think that we might instead of a new API it might be possible to try relying on ILabelProvider and adapters, but this would be more complicated for no strong value and also less encouraging than a new default method in API, so let's stick to a default method).
There was a problem hiding this comment.
I moved the method like you suggested but this seems to be classified as an API breaking change, so I have to bump the version to 4.0.0.
How should I handle the other dependent projects that do not require the new method?
(3.50,5.0.0] or bump the min version also 4.0.0?
There was a problem hiding this comment.
You should add an API filter. Only the minor version should be increased. All hell will break loose if you increment the major version.
There was a problem hiding this comment.
@merks Thanks, I already noticed this. I will figure out how this works with an API filter
There was a problem hiding this comment.
There should be a quick fix that does the work for you. 👍
| * | ||
| * @return name of the Handler | ||
| */ | ||
| String getHandlerName(); |
There was a problem hiding this comment.
Or should this stay in a separate interface as it is optional?
default methods are optional to implement as well.
We don't strongly care about implementation here, we care more about APIs (which we can't easily change later).
I would still recommend that you make it a new default method in IHandler if possible, because it actually seems to provide more value than the current extension interface.
While you do that, I think it'd be better to remame the method to getHandlerLabel() because the name can be interpreted as an "id" which is not the case here. (Thinking more about it, it could be just getLabel() but there would be too high risks of conflicts so let's stick to getHandlerLabel(), and this makes me think that we might instead of a new API it might be possible to try relying on ILabelProvider and adapters, but this would be more complicated for no strong value and also less encouraging than a new default method in API, so let's stick to a default method).
N1k145
force-pushed
the
issue1314
branch
4 times, most recently
from
e0657d7 to
b1c44ec
Compare
| /** | ||
| * Called by the framework to allow the handler to specify a label. | ||
| * | ||
| * @return name of the Handler |
There was a problem hiding this comment.
We need to document here, "can be null`
| @@ -90,4 +91,14 @@ public interface IHandler { | |||
| * performed. | |||
| */ | |||
| void removeHandlerListener(IHandlerListener handlerListener); | |||
|
|
|||
| /** | |||
| * Called by the framework to allow the handler to specify a label. | |||
There was a problem hiding this comment.
"Called by" is an implementation detail. The Javadoc should state more the goal of this method per se. "Return the label for this handler. The handler can implement this method to provide a more dynamic label according to the actual action it performend. When returning null, callers may instead use the invoking command label or a generic label."
There was a problem hiding this comment.
Thanks for the hint, I updated the java doc
Fixes Regression: eclipse-platform#1314 With this a basic support for named handlers is introduced, currently this is for compatibility between ActionHandlers and E4.
|
@mickaelistria @BeckerWdf Thank you for taking a look and for the review. |
|
Thank you |
|
I have these errors in my workspace now:
It should be 3.12, right? It's an API change so there should be an version increment, right? |
|
No problem, I'm creating a PR. |
|
This should address the issue: |
|
@mickaelistria the API check did not want me to increase the version, as we ignored the change. And the commit before this one increased the version. So bad luck in merging and not being up to date with master |
Fixes eclipse-platform#1317 Moving TabFolderLayout into SWT. Incorporated review comments. Fixes eclipse-platform/eclipse.platform.swt#1317
Fixes eclipse-platform#1317 Moving TabFolderLayout into SWT. Incorporated review comments. Fixes eclipse-platform/eclipse.platform.swt#1317
Fixes Regression: #1314
With this a basic support for named handlers is introduced, currently this is for compatibility between ActionHandlers and E4.