Bugzilla – Full Text Bug Listing
| Summary: | [IDE] Add UI preference for new Preference: PREF_LIGHTWEIGHT_AUTO_REFRESH | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | James Blackburn <jamesblackburn+eclipse> | ||||||||||||||||||||
| Component: | UI | Assignee: | Szymon Brandys <Szymon.Brandys> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||||
| Priority: | P3 | CC: | caniszczyk, daniel_megert, markus.kell.r, mober.at+eclipse, prakash, Szymon.Brandys, yevshif | ||||||||||||||||||||
| Version: | 3.7 | Flags: | Szymon.Brandys:
review+
|
||||||||||||||||||||
| Target Milestone: | 3.7 M7 | ||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||
| Bug Depends on: | 303517, 341075 | ||||||||||||||||||||||
| Bug Blocks: | 14867 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
James Blackburn
Created attachment 191914 [details]
ui patch 1
Created attachment 192264 [details]
ui patch 2
Update patch for preference moved to ResourcesPlugin.
Update copyright notice to point to this bug.
Some comments: - New preferences names should be IDEWorkspacePreference_RefreshPollingButtonText and IDEWorkspacePreference_RefreshPollingButtonToolTip without 'Page' in the name. - There is no mnemonic for the new preference. - I would rename fsPollingRefresh to just pollingRefresh and fsPollingRefreshButton to pollingRefreshButton. Looking at the discussion at bug 303517 I can't see any objection for the suggested UI. So when issues from comment 3 are fixed, I'm going to release the fix. Created attachment 192442 [details]
patch 3
Updates as per Szymon's comments.
Thanks James. Fixed. The UI needs some more work: - Mnemonic conflict in new message. Use "Allow file &system polling". - Indent should be hardcoded to 20 pixels for now (see bug 341604). - Enablement/selection state: I had the 3.6 option enabled, and when I opened the preference page in N20110404-2000, the main option was disabled/unselected and the sub-option was enabled/selected. => The main option should never be disabled when the sub-option is enabled. When I clicked the sub-option twice, the main option became selected but was again disabled => Inconsistent, I cannot get back to the original state any more. Created attachment 192548 [details]
patch 4
Fix for Markus' issues.
The desire was that the old full-refresh would force enable the lightweight refresh as well. However as this preference didn't previously exist, and we want to allow products to conditionally enable the preferences separately, it makes sense not to change the control's enablement.
Selecting the old refresh preference still automatically selects the lightweight refresh preference.
(In reply to comment #8) > Created attachment 192548 [details] [diff] > patch 4 Looks good, except that &polling conflicts with &Prompt. Created attachment 192555 [details]
patch 5
Sigh, need to read more carefully...
Sorry, but the patch does not look good. Right now you can set refreshes independently of each other. What you should do is to make sure that "the main option is never disabled when the sub-option is enabled." So you need some code that checks if only polling is set and sets also the new preference. (In reply to comment #11) > "the main option is never disabled when the sub-option is enabled." That means users cannot configure the 3.6 state any more (only polling & file system hook, but no out-of-sync prevention). If that's the idea, then you also need to either - add migration code that enables the new option in a 3.6 workspace iff the old option was enabled, or - make sure the out-of-sync prevention code also checks the old option. Looking closer at the tooltip of the "Allow file system polling" checkbox, I'm not sure if the labels and tooltips are right: The "(where a fast file system hook doesn't exist)" suggests that this option behaves differently whether a file system hook is available or not. But AFAIK, the old (sub-)option toggles both file system polling and file system hook, and the new (main) option only toggles out-of-sync prevention. I think the issue is that core.resources allows to set these two kinds of refresh independently, once in the UI they are synced. So the compatibility code should be done on the UI level. To make it easier and less complicated to explain, I would suggest to change the UI in the following way: option 1) Refresh automatically (Refresh using native hooks or polling) option 2) Refresh on access (Refresh resources on access) There will be no compatibility issue, since these two will be independent in the UI. We may improve wording though. (In reply to comment #13) > option 1) Refresh automatically (Refresh using native hooks or polling) > option 2) Refresh on access (Refresh resources on access) > > There will be no compatibility issue, since these two will be independent in > the UI. We may improve wording though. A slight subtlety is that we want to be able to enable all 'lightweight' refresh mechanisms (such as fs hooks) when the lightweight preference is enabled. This isn't currently implemented, but if we make the UI read: 'Refresh on Access' then we'll have more arguments next year when fs hooks are enabled by this pref... This is why the two preferences really should be considered together or scoped: lightweight + full. Lightweight keeps everything in sync in the best way the WS knows how without enabling CPU burning polling. Full enables everything the previous preference used to enable. When you have the existing refresh selected you really want the workspace to keep your resources in sync. You're implicitly enabling everything lightweight would enable. Changing the preference via a UI migration would seem the best option from my POV. I like the independent preferences because they control two different things (as also the Core API indicates): one preference triggers the refresh from outside even if nothing is currently done in the workspace. The other refreshes when something is done. However, I can also live with the solution where the old (polling) mechanism depends on the new one. In the latter case the names should be improved because as a user I don't see why I should enable polling if I already enabled 'Refresh automatically'.
> I think the issue is that core.resources allows to set these two kinds of
> refresh independently, once in the UI they are synced
This raised the question whether the Core API is OK if we really think they
have to depend on each other at the UI level. What happens if someone sets the
Core preference in a way that is not supported by the UI?
Migration code - if really needed - must be at the Core level otherwise we get
different behavior if I start my workspace with just Core bundles enabled
versus Core + UI enabled.
So, do we agree for two options: option 1) Refresh automatically (Refresh using native refresh hooks where available or polling) option 2) Refresh on access (Refresh resources on access via the workspace) ? If not, please suggest other names/tooltips. (In reply to comment #16) > So, do we agree for two options: > option 1) Refresh automatically (Refresh using native refresh hooks where > available or polling) > option 2) Refresh on access (Refresh resources on access via the workspace) +1 but without the text in parenthesis as tool tip. (In reply to comment #17) > (In reply to comment #16) > > So, do we agree for two options: > > option 1) Refresh automatically (Refresh using native refresh hooks where > > available or polling) > > option 2) Refresh on access (Refresh resources on access via the workspace) > +1 but without the text in parenthesis as tool tip. That should have read: +1 but with the text in parenthesis as tool tip (not in the label). I'm concerned about two things: - Understanding the difference between the two preferences requires the user to understand core.resources implementation details. - In 3.8 we will make the preference backing 'refresh on access' enable fs hooks as well. (In reply to comment #19) > I'm concerned about two things: > - Understanding the difference between the two preferences requires the > user to understand core.resources implementation details. How about this: [ ] Refresh on external changes [ ] Refresh on access > - In 3.8 we will make the preference backing 'refresh on access' enable fs > hooks as well. In 3.8 we *might* then change this to: [ ] Refresh automatically [ ] Allow polling (In reply to comment #20) > How about this: > [ ] Refresh on external changes > [ ] Refresh on access It looks also good to me. Changing the meaning of a preference later on is problematic. Can't you do the change right now, i.e. - PREF_LIGHTWEIGHT_AUTO_REFRESH: enable fs hooks and refresh on access - PREF_AUTO_REFRESH: enable polling and fs hooks Then the UI could look like this: [ ] Refresh automatically (Tooltip: Refreshes files on access and using a fast file system hook, Preference: PREF_LIGHTWEIGHT_AUTO_REFRESH) [ ] Refresh periodically (Tooltip: Refreshes the workspace periodically using polling or file system hook, Preference: PREF_AUTO_REFRESH) If the fs hook can only be added to PREF_LIGHTWEIGHT_AUTO_REFRESH in 3.8, then you could still use the given labels but set the first tooltip to "Refreshes files on access" for 3.7. > Then the UI could look like this:
> [ ] Refresh automatically
> [ ] Refresh periodically
This would look strange to the user and tool tips don't help too much as they are not common on preference page labels.
(In reply to comment #23) > This would look strange to the user and tool tips don't help too much as they > are not common on preference page labels. But the existing proposal: > How about this: > [ ] Refresh on external changes > [ ] Refresh on access gives the wrong impression too. - "Refresh on external changes" makes it sound like work only happens when changes are made externally. (untrue: polling happens periodically irrespective of external changes) - "Refresh on access" sounds like additional work happens every time a file is 'accessed'. (untrue: work only happens when a file is discovered out-of-sync. The out-of-sync check happens on every access anyway.) I would still argue that the user will always want LIGHTWEIGHT_REFRESH on. However if we must have a preference for this, the new proposed naming seems to make lightweight_refresh sound more heavyweight than polling refresh. (In reply to comment #24) What do you think about more descriptive labels, e.g.: [] Refresh automatically via/using native hooks or polling [] Refresh on access (In reply to comment #25) > (In reply to comment #24) > What do you think about more descriptive labels, e.g.: > > [] Refresh automatically via/using native hooks or polling > [] Refresh on access or: [] Refresh using native hooks or polling [] Refresh on access Created attachment 193237 [details] patch (comment 26) Created attachment 193238 [details] Screenshot (comment 26) Created attachment 193240 [details] patch 2 (comment 26) Created attachment 193241 [details] patch 3 (comment 26) minor wording change in the tooltip I used Dani's wording in the latest patch. The patch released to HEAD. |