Community
Participate
Working Groups
I20051214-2000 - start with a new workspace and a 1.4 VM - on the 'Installed JREs' page, add a 1.5 SDK - without leaving the preferences dialog, go to the 'Execution Environments' page and select J2SE-1.5 => compatible JREs list is empty, although I just installed one
The page should udpate when JREs are added/removed (but the user will have to press Apply on the installed JREs page for this to happen).
Deferred
*** Bug 143914 has been marked as a duplicate of this bug. ***
*** Bug 144391 has been marked as a duplicate of this bug. ***
*** Bug 174239 has been marked as a duplicate of this bug. ***
Any insight as to why this is resolved/later? As I mentioned in my now duplicate report, this is essentially the inverse of bug 120676, which was fixed in the EnvironmentsManager.
This may require API changes and is not a trivial fix. The execution envirnments/JRE infrastructure is not designed to handle JRE's that are not 'part' of the workspace yet. When a JRE is added on the JRE page, it is not committed to the workspace until the page's changes are applied. The EE page works on JREs that are visible to the workspace, not those that are being created/edited in the preferences. It's not critical, and there is a simple workaround - press 'OK' on the JRE page and then re-open the pref dialog to the EE page.
*** Bug 181456 has been marked as a duplicate of this bug. ***
*** Bug 183205 has been marked as a duplicate of this bug. ***
*** Bug 215456 has been marked as a duplicate of this bug. ***
Re-opening, as this seems to be a common problem for users.
Marking as 3.5 candidate
(In reply to comment #7) > The execution > envirnments/JRE infrastructure is not designed to handle JRE's that are not > 'part' of the workspace yet. To fix bug 246239 in JDT/UI, we would also need access to these "working copy" JREs, unless JavaRuntime.getDefaultVMInstall() returns the new setting immediately.
I haven't looked into the code of the EE page yet, but I bet we could use preferences working copy manager to fix this: that way we could 'know' about uncommitted changes between the pages.
I removing this from the 3.5 plan. It would be nice to have this enhancement but we currently have no resources to work on the feature. Contributions would be accepted/reviewed.
*** Bug 263886 has been marked as a duplicate of this bug. ***
A simpler fix would be to just apply the changes when the user leaves the Installed JREs page, and reload the Execution Environments page each time it becomes visible. That's how e.g. BuildPathsPropertyPage#setVisible(..) does it, see bug 32378 comment 21.
If there would be a way to only show the Apply button, but not Restore Defaults (bug 81202), then the user could at least do it manually without having to hunt for the Preferences dialog again.
(In reply to comment #17) > A simpler fix would be to just apply the changes when the user leaves the > Installed JREs page, and reload the Execution Environments page each time it > becomes visible. Applying changes can trigger builds and build paths (JRE containers) to be re-bound. If we perform apply when they leave the page, what happens if they eventually press cancel?
>If we perform apply when they leave the page, what happens if they >eventually press cancel? I think Markus meant that the user can press 'Apply'. At this point it doesn't matter what he does afterwards as it means to commit the current values.
The Windows UI guidelines say that Apply should store settings permanently, even if the user cancels later. That's also what Eclipse does. Mac and GTK guidelines would mandate to revert on Cancel, but we follow Windows in this case.
*** Bug 280408 has been marked as a duplicate of this bug. ***
*** Bug 319093 has been marked as a duplicate of this bug. ***
*** Bug 352401 has been marked as a duplicate of this bug. ***
I think this is mostly a discoverability issue. It's not immediately obvious from the flow of the Installed JREs pane that the changes aren't committed until you commit the Preferences dialog. In many similar configuration tools where you make changes in a sub-dialog the changes are committed when you commit the sub-dialog. As Markus mentioned in comment 18, showing an enabled Accept button on the Installed JREs pane would provide a clear affordance that the changes haven't been committed.
*** Bug 401438 has been marked as a duplicate of this bug. ***
Still a usability issue. If one knows of this problem it's ok, but if one doesn't he/she may be confused on how the Execution Environment preferences page works, since the newly added JREs are not shown.
We should simply add the 'Apply' and the 'Restore Defaults' buttons i.e. remove noDefaultAndApplyButton() from JREsPreferencePage#createContents(Composite). While we get the apply behavior for free, the only thing to add is the restore behavior: simply remove all JREs and add the one used for launching the workspace.
(In reply to comment #28) > We should simply add the 'Apply' and the 'Restore Defaults' buttons i.e. > remove noDefaultAndApplyButton() from > JREsPreferencePage#createContents(Composite). While we get the apply > behavior for free, the only thing to add is the restore behavior: simply > remove all JREs and add the one used for launching the workspace. Funnily enough, Curtis and I were talking about this exact same thing yesterday, except that we thought 'Restore Defaults' should change the default JRE back to the original, rather than delete any JREs
(In reply to comment #29) > (In reply to comment #28) > > We should simply add the 'Apply' and the 'Restore Defaults' buttons i.e. > > remove noDefaultAndApplyButton() from > > JREsPreferencePage#createContents(Composite). While we get the apply > > behavior for free, the only thing to add is the restore behavior: simply > > remove all JREs and add the one used for launching the workspace. > > Funnily enough, Curtis and I were talking about this exact same thing > yesterday, except that we thought 'Restore Defaults' should change the > default JRE back to the original, rather than delete any JREs No, it should delete the manually added ones. But, there's one thing we need to consider: JREs added via plug-in/product customization must also be added when restoring the defaults.
(In reply to comment #30) > (In reply to comment #29) > > (In reply to comment #28) > > > We should simply add the 'Apply' and the 'Restore Defaults' buttons i.e. > > > remove noDefaultAndApplyButton() from > > > JREsPreferencePage#createContents(Composite). While we get the apply > > > behavior for free, the only thing to add is the restore behavior: simply > > > remove all JREs and add the one used for launching the workspace. > > > > Funnily enough, Curtis and I were talking about this exact same thing > > yesterday, except that we thought 'Restore Defaults' should change the > > default JRE back to the original, rather than delete any JREs > > No, it should delete the manually added ones. But, there's one thing we need > to consider: JREs added via plug-in/product customization must also be added > when restoring the defaults. Well, actually that does not work at all yet (see bug 241278).
(In reply to comment #30) > No, it should delete the manually added ones. But, there's one thing we need > to consider: JREs added via plug-in/product customization must also be added > when restoring the defaults. We have no way to easily know what was manually added, plus to be able to delete said JREs we would first have to save back to JavaRuntime (to flush the VM cache) which would cause mass re-binding (EEs) / building in the workspace prior to being able to ask JavaRuntime to recompute the system defaults. So I still maintain my position on not deleting anything.
(In reply to comment #32) > (In reply to comment #30) > > > No, it should delete the manually added ones. But, there's one thing we need > > to consider: JREs added via plug-in/product customization must also be added > > when restoring the defaults. > > We have no way to easily know what was manually added, Yes, I know that. Just wanted to draw the picture here. > plus to be able to > delete said JREs we would first have to save back to JavaRuntime (to flush > the VM cache) which would cause mass re-binding (EEs) / building in the > workspace prior to being able to ask JavaRuntime to recompute the system > defaults. So I still maintain my position on not deleting anything. Restore Defaults should do the same as everywhere i.e. it must look like starting a fresh workspace. Note that the user first sees the change and it would only be applied when he presses Apply or OK. So, give the user what he asked for.
(In reply to comment #33) > (In reply to comment #32) > Restore Defaults should do the same as everywhere i.e. it must look like > starting a fresh workspace. Note that the user first sees the change and it > would only be applied when he presses Apply or OK. So, give the user what he > asked for. I could probably make this work. I'll take a swing at it for M2.
*** Bug 419993 has been marked as a duplicate of this bug. ***
Can't we have something similar to what we do when leaving the Java Build Path Preference Page ? So that user knows before leaving the page that he should Apply or Discard the Changes ?
(In reply to Sarika Sinha from comment #36) > Can't we have something similar to what we do when leaving the Java Build > Path Preference Page ? So that user knows before leaving the page that he > should Apply or Discard the Changes ? We definitely could. We could also just add our own apply button so we don't have to deal with the defaults button - leave the noDefaultAndApplyButton() and just create our own button.
Created attachment 248562 [details] Addition of only Apply button in Preference Page
Created attachment 248563 [details] Debug patch to support Apply in JREs Preference page
Created attachment 248564 [details] UI patch to support Apply in Build Path Preference page
Enhanced Preference Page to support addition of only Apply button (without Default button). Have added Apply button in "Installed JREs" and "Build Path" Preference pages. Have also added a warning message to be displayed to show unsaved on moving out of "Installed JREs" page without applying the changes.
(In reply to Sarika Sinha from comment #41) > Enhanced Preference Page to support addition of only Apply button (without > Default button). Have added Apply button in "Installed JREs" and "Build > Path" Preference pages. Have also added a warning message to be displayed to > show unsaved on moving out of "Installed JREs" page without applying the > changes. 1. Rather than add new API to PreferencePage, could you not just contribute our own apply button using org.eclipse.jface.preference.PreferencePage.contributeButtons(Composite)? 2. In InstalledJREsBlock could you not get rid of all the timestamp code and simply set a boolean when changes to the backing list of vms is made (to determine dirty state)? That way the dirty state uses the same logic we use to actually update the VMs when you close the page. 3. the warning string is a bit wordy, couldn't 'The Installed JREs property page contains unsaved modifications. Do you want to save changes so that other Installed JREs related property pages can be updated?' be simplified to something like: 'The page contains unsaved JRE modifications.'
Comment on attachment 248564 [details] UI patch to support Apply in Build Path Preference page I would use the same naming for the new API as already used to hide the Apply and Default button ==> #noDefaultButton(). To make the code more readable I'd replace 'createDefaultAndApplyButton' with 'createApplyButton' and 'createDefaultButton', and adapt the code accordingly. This will also make the button creation code more readable. Please remove all unrelated whitespace changes.
(In reply to Michael Rennie from comment #42) > (In reply to Sarika Sinha from comment #41) > > Enhanced Preference Page to support addition of only Apply button (without > > Default button). Have added Apply button in "Installed JREs" and "Build > > Path" Preference pages. Have also added a warning message to be displayed to > > show unsaved on moving out of "Installed JREs" page without applying the > > changes. > > 1. Rather than add new API to PreferencePage, could you not just contribute > our own apply button using > org.eclipse.jface.preference.PreferencePage.contributeButtons(Composite)? No, that would be wrong: There is already API to hide both buttons, so, adding API to only hide one of them makes sense. Also, first asking not to create the 'Apply' button and then add it in the client code isn't really a good idea ;-).
(In reply to Michael Rennie from comment #42) > 2. In InstalledJREsBlock could you not get rid of all the timestamp code and > simply set a boolean when changes to the backing list of vms is made (to > determine dirty state)? That way the dirty state uses the same logic we use > to actually update the VMs when you close the page. I'm fine either way here. We successfully use the time stamp in the other case, so, using it here would be OK for me. > 3. the warning string is a bit wordy, couldn't We should use the same wording for both cases. More is better here, I think. Sarika, what do you think? One thing I noticed is that clicking the [x] in the dialog leaves us on the new preference/property page. I would expect to stay on the previous page, since it is not clear which action we took.
(In reply to Dani Megert from comment #45) > > One thing I noticed is that clicking the [x] in the dialog leaves us on the > new preference/property page. I would expect to stay on the previous page, > since it is not clear which action we took. Same thing happens for Java Build Path. We should change for both or let it remain as it is now.
How about having warning string like : 'This page contains unsaved modifications. Do you want to save changes so that other related pages can be updated?'
Created attachment 248892 [details] Addition of only Apply button in Preference Page
Created attachment 248893 [details] Debug patch to support Apply in JREs Preference page
Created attachment 248894 [details] UI patch to support Apply in Build Path Preference page
Comment on attachment 248892 [details] Addition of only Apply button in Preference Page The Javadoc of all new members is wrong.
(In reply to Sarika Sinha from comment #47) > How about having warning string like : > > 'This page contains unsaved modifications. Do you want to save changes so > that other related pages can be updated?' I would make the connection to the 'Apply' button ==> This page contains unsaved modifications. Do you want to apply the changes so that other related pages can be updated?
(In reply to Dani Megert from comment #52) > (In reply to Sarika Sinha from comment #47) > > How about having warning string like : > > > > 'This page contains unsaved modifications. Do you want to save changes so > > that other related pages can be updated?' > > I would make the connection to the 'Apply' button ==> > > This page contains unsaved modifications. Do you want to apply the changes > so that other related pages can be updated? Actually, this is not working, because at the time we show the dialog, the page is already the new one. Hence "this page" is not enough and we need to have the name of the old page in the message and the title.
(In reply to Sarika Sinha from comment #46) > (In reply to Dani Megert from comment #45) > > > > One thing I noticed is that clicking the [x] in the dialog leaves us on the > > new preference/property page. I would expect to stay on the previous page, > > since it is not clear which action we took. > > Same thing happens for Java Build Path. We should change for both or let it > remain as it is now. Let's leave this as is for now.
Created attachment 249124 [details] Warning Dialog
(In reply to Dani Megert from comment #53) > (In reply to Dani Megert from comment #52) > > (In reply to Sarika Sinha from comment #47) > > > How about having warning string like : > > > > > > 'This page contains unsaved modifications. Do you want to save changes so > > > that other related pages can be updated?' > > > > I would make the connection to the 'Apply' button ==> > > > > This page contains unsaved modifications. Do you want to apply the changes > > so that other related pages can be updated? > > Actually, this is not working, because at the time we show the dialog, the > page is already the new one. Hence "this page" is not enough and we need to > have the name of the old page in the message and the title. Attached the screenshot of dialog, it stays in Installed JREs Page and the dialog heading also mentions the same.
(In reply to Sarika Sinha from comment #56) > (In reply to Dani Megert from comment #53) > > (In reply to Dani Megert from comment #52) > > > (In reply to Sarika Sinha from comment #47) > > > > How about having warning string like : > > > > > > > > 'This page contains unsaved modifications. Do you want to save changes so > > > > that other related pages can be updated?' > > > > > > I would make the connection to the 'Apply' button ==> > > > > > > This page contains unsaved modifications. Do you want to apply the changes > > > so that other related pages can be updated? > > > > Actually, this is not working, because at the time we show the dialog, the > > page is already the new one. Hence "this page" is not enough and we need to > > have the name of the old page in the message and the title. > > Attached the screenshot of dialog, it stays in Installed JREs Page and the > dialog heading also mentions the same. Looks good! My workspace must have been in a strange state. When testing I saw this: > > when we show the dialog, the page is already the new one. Testing today, I see the same as you: the dialog is shown on top of the page we want to leave.
Created attachment 249125 [details] Addition of only Apply button in Preference Page
Created attachment 249126 [details] Debug patch to support Apply in JREs Preference page
Created attachment 249127 [details] UI patch to support Apply in Build Path Preference page Updated Java Doc and changed to "Apply" in dialog message.
Comment on attachment 249125 [details] Addition of only Apply button in Preference Page The Javadoc for createApplyButton is still wrong. We should avoid the code duplication of the Apply button creation. I would expect something like this: if (createDefaultButton) { create Default button code } if (createApplyButton) { create Apply button code }
Created attachment 249277 [details] UI patch to support Apply in Build Path Preference page Approach 1
Created attachment 249279 [details] UI patch to support Apply in Build Path Preference page Approach 2 Added 2 approaches to remove duplicate code in PreferencePage
(In reply to Sarika Sinha from comment #62) > Created attachment 249277 [details] [diff] > UI patch to support Apply in Build Path Preference page Approach 1 This is fine but you must not reuse the same GridData data instance. Also, the column size can be expressed easier: layout.numColumns = 1 + (createApplyButton && createDefaultButton ? 1 : 0); (In reply to Sarika Sinha from comment #63) > Created attachment 249279 [details] [diff] > UI patch to support Apply in Build Path Preference page Approach 2 > > Added 2 approaches to remove duplicate code in PreferencePage #applyDialogFont should not be part of #createApplyButton. This approach still has code duplication and will fail if we introduce noApplyButton() at some point. Therefore we will go with approach 1. Please update the patch for approach 1 with my comments and then we're ready to go with that change.
Created attachment 249458 [details] UI patch to only show 'Apply' button in PreferencePage Approach 1 Updated Approach 1 patch accordingly.
(In reply to Sarika Sinha from comment #65) > Created attachment 249458 [details] [diff] > UI patch to support Apply in Build Path Preference page Approach 1 > > Updated Approach 1 patch accordingly. Committed via bug 81202.
Comment on attachment 249126 [details] Debug patch to support Apply in JREs Preference page #getEncodedSettings The name is misleading. Usually we use "settings" for dialog settings. I would rename this to #getEncodedVMInstalls. The code to write out the default VM is redundant to the code that writes the attributes. Simply mark the default VM when encoding all the VMs. In the loop add: if (elem == vmInstall) { // marks as default; } #initializeTimeStamp does not need to be protected #okToLeave should use the wording "apply" not "save" in the comments With those changes the patch is good to go.
(In reply to Dani Megert from comment #67) > With those changes the patch is good to go. Actually there's a small issue when the 'Execution Environment' page already has a selection: - select an entry - go to the JRE page - add or remove a JRE - switch to the EE page and apply ==> the EE page shows old content The same problem happens when going from JRE to Compiler preference page: the warning/status is only updated when the Compiler page hasn't been shown before.
(In reply to Dani Megert from comment #68) > (In reply to Dani Megert from comment #67) > > With those changes the patch is good to go. > > Actually there's a small issue when the 'Execution Environment' page already > has a selection: > - select an entry > - go to the JRE page > - add or remove a JRE > - switch to the EE page and apply > ==> the EE page shows old content > > The same problem happens when going from JRE to Compiler preference page: > the warning/status is only updated when the Compiler page hasn't been shown > before. Created Bug 455556 for this. Will need the support from Platform UI to make this work in EE and Compiler page.
Created attachment 249512 [details] Debug patch to support Apply in JREs Preference page Review comments incorporated !!
(In reply to Sarika Sinha from comment #70) > Created attachment 249512 [details] [diff] > Debug patch to support Apply in JREs Preference page > > Review comments incorporated !! #initializeTimeStamp is now opened it up even more. I've fixed that before committing your latest patch. Committed with http://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/commit/?id=2ef686649486e20d93522f435f6d4c4c66fa1cb9 I've also removed the superfluous semicolon that gave a compile warning.
As long as bug is not fixed, this bug here can't be marked fixed.
(In reply to Sarika Sinha from comment #69) > Created Bug 455556 for this. Will need the support from Platform UI to make > this work in EE and Compiler page. I don't think so. What we need to do is recompute the page based on the applied state. This can be done when setVisible(visible) is called with visible == true.
Fixed EE refresh page issue via http://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/commit/?id=d3f82f52c5976cec08019bc7682ff3c1afd4bf44 For Compliance page, need to discuss with JDT UI team as we already have the required code but checks for (fProject != null) before updating Compliance warning.
(In reply to Sarika Sinha from comment #74) > For Compliance page, need to discuss with JDT UI team as we already have the > required code but checks for (fProject != null) before updating Compliance > warning. Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=8953803ece6cec9545d597ff037c58328791bc89
Sarika, please verify that this now works as expected.
Verified using Eclipse SDK Version: Mars (4.5) Build id: I20150116-1000
