-
Notifications
You must be signed in to change notification settings - Fork 184
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
Workbench restart is unavailable if --launcher.noRestart is used #2125
Workbench restart is unavailable if --launcher.noRestart is used #2125
Conversation
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 haven't tested it. Not sure how to build a new IDE with Equinox I-build and this PR both included.
To test this you can for example create a new Eclipse Application
launch configuration based on Features and only add the org.eclipse.sdk
feature. Then in the arguments tab you can simply add --launcher.noRestart
as program argument.
I have made some comments below for a few enhancements.
In general instead of informing the user via a Error-Dialog or an error log entry just an UnsupportedOperationException
could be thrown. But the current variant is probably more friendly.
In order to disable the File -> Restart menu entry you should also add the following constructor to org.eclipse.ui.internal.handlers.RestartWorkbenchHandler
:
public RestartWorkbenchHandler() {
setBaseEnabled(E4Workbench.canRestart());
}
bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/workbench/IWorkbench.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench/E4Workbench.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench/E4Workbench.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/messages.properties
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/messages.properties
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/Workbench.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/Workbench.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/Workbench.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/Workbench.java
Outdated
Show resolved
Hide resolved
Is there a specific Oomph setup for Eclipse UI? Will that then pull in the latest I-build with the new launcher?
Indeed, that was the idea. I kept it.
The problem with that, is that a user doesn't know why it is disabled. Hence, I opted to keep it enabled, but instead give a message to the user that explains why it can't be completed. |
d84c3ec
to
f5c092a
Compare
I made all the changes, as best I could. I squashed the commit. Ready for next review. |
This setup is a bit overkill that it includes everything but if you back up the wizard to the Projects page you can deselect projects you don’t need/want. It installs the nightly Ibuild and populates the target platform with that as well. |
f5c092a
to
609b15d
Compare
I broke some import statements, but fixed them now. |
I must confess that from a Users Experience Point of View it does not work correctly. If the action is not available it should either:
Also the message is to much technical for a user to understand (too much information) and unlikely to be actionable. |
It works as designed, but apparently you're looking for a different design.
Not showing it has the downside that a user then does not know why it is not shown, when normally it is shown. Similar with disabling it, as was suggested before. You suggest it is possible to disable, but have a tooltip text that explains why it is not shown? That would be nice. But, I've never heard of special disabled text tooltips. How does one set those? |
It depends, there are many things that are not shown in Eclipse, so this is the usual behavior. At least I have never used this Restart function anyways in > 10 years of Eclipse so some even won't notice that. In contrast to that, I now get a hard to understand error popup that indicates I should remove the option I carefully have put into my product configuration before... but why have I put it there if I want to support restarts?!?
Yes, I have not checked if its possible. |
609b15d
to
28b4cfb
Compare
21c2235
to
d6445c9
Compare
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.
Thank you @dhendriks for the update and of course this PR in general.
In contrast to that, I now get a hard to understand error popup that indicates I should remove the option I carefully have put into my product configuration before... but why have I put it there if I want to support restarts?!?
I also agree that first of all we should preventing restarts from being possible. A dedicated tool-tip for the disabled state would of course be nice in this case, but with the given use-case I think it's also fine to not have it.
As we already said in the very beginning the new --launcher.noRestart
option is not intended for UI applications anyway. And if one stil adds them to a config, that person should remember that we noticing the disabled Restart entry. And if still someone adds that option to a product those devs should probably take care on their own to handle it better (e.g. add the tool-tip too).
Even though restart is now disabled if that option is present there can still be cases where the dialog can pop-up. I'm fine with that (again since it should actually not be used I'm also fine with a not 100% perfect solution) but tried to further enhance the readability of the text in the dialog.
With that I'm fine with this change. If there are no objections I'll submit this tomorrow evening.
d6445c9
to
1ef0458
Compare
I changed the logged message in |
Thanks. Also for all your contributions on this topic. It was great work. If you are interested in contributing more to Eclipse, that would be welcome. :) |
This new argument just got added. See eclipse-equinox/equinox#595. Here we make sure the workbench understands it.
Should be rare that people actually use the option and start the IDE, as it is not meant for that. This PR detects it and informs the user restart is not available, as documented (see eclipse-platform/eclipse.platform.releng.aggregator#2195).
I haven't tested it. Not sure how to build a new IDE with Equinox I-build and this PR both included.