Bug 184430 - "Check for External Changes" progress shall not hide other (more important) progresses
"Check for External Changes" progress shall not hide other (more important) p...
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Progress
6.x
Other Linux
: P1 (vote)
: 6.x
Assigned To: David Simonek
issues@platform
: API_REVIEW_FAST, PERFORMANCE, UI
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-19 10:15 UTC by Jaroslav Tulach
Modified: 2010-06-01 07:37 UTC (History)
11 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
(re)uses the netbeans.indexing.noFileRefresh property; reuses existing ErrorAnnotator + reuse existing refresh action; does FileUtil.refreshAll asynchronously (7.14 KB, patch)
2010-04-19 10:15 UTC, Jaroslav Tulach
Details | Diff
Updated; documented (9.76 KB, patch)
2010-04-19 14:26 UTC, Jaroslav Tulach
Details | Diff
Ondřej's UI proposal (179.55 KB, image/png)
2010-04-27 11:18 UTC, Jaroslav Tulach
Details
Adjusting to UI spec and VS02 (31.72 KB, patch)
2010-04-27 13:05 UTC, Jaroslav Tulach
Details | Diff
Screenshot of the first dialog; I don't have the screenshot of the second dialog right now (20.93 KB, image/png)
2010-05-03 13:35 UTC, Petr Jiricka
Details
screenshot of the dialog for disabling the scan for external changes (25.65 KB, image/png)
2010-05-03 15:40 UTC, Tomas Pavek
Details
Change to progress UI (1.51 KB, patch)
2010-05-12 14:14 UTC, Jaroslav Tulach
Details | Diff
incorrect labels (17.36 KB, image/png)
2010-05-18 14:52 UTC, Vladimir Voskresensky
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Tulach 2010-04-19 10:15:23 UTC
Created attachment 97605 [details]
(re)uses the netbeans.indexing.noFileRefresh property; reuses existing ErrorAnnotator + reuse existing refresh action; does FileUtil.refreshAll asynchronously

To eliminate the recent complains about too frequent scan for external changes (trigger mostly by P1 error #182801), it has been decided to push further our effort to allow user to manually control the rescan.

The user scenario:
1. user is bothered by external refresh and decides to get rid of it
2. user clicks cancel on the progress and gets warning message
3. after confirming the warning, the refresh on window focus is disabled
4. user can trigger the refresh manually. Either globally Source/Scan for... or locally by popup menu on a folder

I am attaching the simplest implementation I can think of that satisfies the given goals.
Comment 1 Tomas Zezula 2010-04-19 10:27:14 UTC
Is there a way how the user can restore the default behavior (refresh after focus gained)?
Comment 2 Jan Lahoda 2010-04-19 10:46:21 UTC
(Not saying that I agree with this, just some comments.)
-I see no persistence - the behavior will revert to default on each start. Is this what we want?
-There should be "No, do not bother me again" option in the dialog
-I see no message being printed to log when the FS refresh is disabled. There must be a message in messages.log.
-ErrorAnnotator.actions - isn't this method called in AWT on the critical path when the pop-up is to be shown? Is it really good to introduce new reflection calls in this path? Why not simply SystemAction.get(FSRA.class), or something similar?
Comment 3 Jaroslav Tulach 2010-04-19 14:26:12 UTC
Created attachment 97631 [details]
Updated; documented

Re. "no persistence". We discussed that today (with Tomáš Z. and David Š.) and guys just had to agree with my assessment: The original decision (about persistence being on) had been made before we knew bug 182801. Now when we believe that the reason for so big user noise was bug 182801, we no longer need to lead people away from refresh check being done regularly.

As a consequence of the above: no need to restore the behavior (just restart), no need for "don't show again" option (press once it per session or adjust netbeans.conf).

The overall plan: When a bug report appears, tell the users to add -J-Dnetbeans.indexing.noFileRefresh=true in etc/netbeans.conf file. If forth report appears enhance an FAQ wiki page. Meanwhile gather data via UI gestures collector and evaluate them for post 6.9 version.

Re. "message being printed to log" - printing an INFO message once per session. 

Re. "ErrorAnnotator.actions" - OK, I did not realize the action is in API.
Comment 4 Vitezslav Stejskal 2010-04-19 19:13:08 UTC
VS01 - ScanForExternalChanges action should not call FileUtil.refreshAll() explicitly. RepositoryUpdater does the same in more efficient way.

VS02 - I'm against tying Parsing & Indexing API to core.ui the way how it is done in the patch. Refreshing filesystems when the main window (re)gains focus is a feature implemented in the platform. If there is a property that controls this feature its behavior should IMO be solely implemented in the platform. In the patch half of this (no filesystem refresh) is in the platform and half of it (manual Refresh action on folders) is in Parsing & Indexing, which is in the IDE cluster. This may be acceptable in the IDE, but IMO is not well designed for other platform apps. And even in the IDE it's a strange interdependency.
Comment 5 Jaroslav Tulach 2010-04-23 10:04:40 UTC
Re. VS01: I am afraid that without refreshAll you would not be notified about new files. Here is a quote from one of emails exchanged between me and Tomáš Zezula:

>> What if we disable the refresh on focus of main window. What if  
>> someone
>> creates new .java file externally in one of the visible projects? We  
>> instruct
>> such users to go to Sources/Scan for External Changes. Well, but  
>> that only
>> iterates over existing FileObject, right? But filesystem's refresh is
>> disabled, so filesystems are not going to notice the new .java file.
>>
>> This may be interpreted as following: Source/Scan for External  
>> Changes needs
>> to perform FileUtil.refreshAll. Right?
>
> Right, we need to force the FS to do the update of metadata.
Comment 6 Vitezslav Stejskal 2010-04-23 11:00:32 UTC
(In reply to comment #5)
> Re. VS01: I am afraid that without refreshAll you would not be notified about
> new files.

I don't think so, RepositoryUpdater.RefreshWork *calls* FileUtil.refreshAll(). There is no need for the ScanForExternalChanges action to call it explicitly. Moreover, RU.RefreshWork calls the FU.rAll() in FS.AtomicAction and ignores any fs events fired from that AA in order not to rescan files twice.
Comment 7 John Jullion-ceccarelli 2010-04-23 13:09:46 UTC
Talked about this with Ondra on Wed and Jarda today. Seems like the main UI question that is still out there is whether to persist this across sessions and add an option to the Options window or whether to just disable scanning for the present session and have users who want to persist it do so with a netbeans.conf switch.

My 2c - users will expect the scanning to be turned off permanently and users who have these scanning problems are more likely to have them all the time, as it has to do with the setup/size of their projects. 

Anyway, we should get this resolved soon (Mon-Tues) so we can get this into the builds next week.
Comment 8 Jaroslav Tulach 2010-04-27 11:18:47 UTC
Created attachment 98121 [details]
Ondřej's UI proposal
Comment 9 Jaroslav Tulach 2010-04-27 13:05:08 UTC
Created attachment 98137 [details]
Adjusting to UI spec and VS02
Comment 10 Jaroslav Tulach 2010-04-27 13:06:28 UTC
Unless there are strong objections, I'll integrate tomorrow.
Comment 11 Petr Jiricka 2010-04-27 20:40:52 UTC
So can you please recap what the "user scenario" from the original description of this task looks like now?

Currently, when I press Yes on the "Are you sure you want to cancel Checking for external changes?" confirmation dialog, just the currently running check is canceled, nothing else. Will this change? Thanks.
Comment 12 Jaroslav Tulach 2010-04-28 12:30:45 UTC
core-main#f285f9922f57
Comment 13 Petr Jiricka 2010-04-29 11:48:30 UTC
Hi, I am trying out the build with Jarda's change and I have a few comments + questions (probably for Ondra):

1. When I cancel the check for external changes, I get the second dialog asking whether I want to turn it off permanently. However, this dialog does not tell me how to turn it back on if I want to re-enable it in the future. Shouldn't the dialog tell me about the setting in IDE Options -> Miscellaneous -> Files?

2. When I answer No in the second dialog (meaning I only want to cancel the check this time, but not in the future), I do not get the dialog next time. So if the user misses this opportunity, but later discovers that they want to turn off the check, they have a discoverability problem.

3. Isn't it weird to get two dialogs in a row? Couldn't they be merged into one? I.e. something like:

---------------------
You are about to cancel Checking for external changes.

You can either cancel the current check only, or disable checks permanently. If you disable checks permanently, you can re-enable them later in the IDE Options dialog, in the Miscellaneous | Files category.

[ ] Do not display this dialog next time

[Cancel this check only] [Disable checks permanently] [Continue check for external changes]
---------------------

4. If the user does not want to disable the check permanently, but does want to cancel it from time to time, they may get annoyed by getting the confirmation dialog every time. This could be improved by adding a [ ] Do not display this dialog next time checkbox, see above.

What do you think?
Comment 14 Jaroslav Tulach 2010-04-29 13:35:29 UTC
#3 will be blocked by missing API. There is no way to pass own confirmation dialog when constructing ProgressHandle. In case Ondřej decides this deserves to be implemented (and I agree it should), let's track it as new issue with dependency on the necessary API change.
Comment 15 Ondrej Langr 2010-04-29 21:57:06 UTC
Jarda, based on review by docs team, could you please change the label text to: "Enable auto-scanning of sources" and remove the hint below? 

Also, could you let me know the help ID for that dialog? Implications of what the option means for the end user will be provided there.
Comment 16 John Jullion-ceccarelli 2010-05-03 11:56:21 UTC
can someone please post a screenshot of the dialog as it now exists? can't force it to come up on my system.
Comment 17 Petr Jiricka 2010-05-03 13:35:57 UTC
Created attachment 98368 [details]
Screenshot of the first dialog; I don't have the screenshot of the second dialog right now
Comment 18 Tomas Pavek 2010-05-03 15:40:26 UTC
Created attachment 98379 [details]
screenshot of the dialog for disabling the scan for external changes

Here's the dialog. Note the "first" dialog is a standard dialog for stopping a task (standard NB progress UI).
Comment 19 Vladimir Voskresensky 2010-05-07 08:26:47 UTC
Guys, I'd like to return to this issue. I'm concerned with UI part of "checking for external changes" progress bar only.
My concern is: never ending progress bar in the status bar.
I see several issues there:
1) "shecking for external changes" progress bar is always on top (visible in status bar) even when other much more important tasks in progress => user have to always look at unimportant progress and can not see progress (and even existence) of important tasks until not click on it to check if other tasks are in place.
2) when I debug application I always switch between NB IDE and app => dozen of window activated events and each give me progress bar
3) we all are afraid of "Scanning in progress" progress, because it usually mean - unusable IDE features in Java area

What I propose:
- get rid of progress bar showing "checking for external changes" activity
- if you *really* need to show this activity for user => use yellow/green ball in the right most corner.
Comment 20 Jaroslav Tulach 2010-05-07 08:47:13 UTC
I guess Vladimir's request formulated as "check for external changes shall not hide other important progresses" is valid. I was also surprised by this behavior.

My code calls ProgressHandle.suspend and in spite of that the progress area often shows status of my ProgressHandle and not any other, not suspended ProgressHandles.

I guess the fix is quite simple. Rather than showing "Suspended" in the progress area, check whether there is another running progress and switch the view to it. That shall be simple change in the implementation (no API changes), can you make it for 6.9 Dafe?

If there is another way I can make the progress less visible (createSystemHandle?), feel free to recommend it and I'll use such existing API.
Comment 21 Vladimir Voskresensky 2010-05-07 09:35:06 UTC
(In reply to comment #20)
> I guess Vladimir's request formulated as "check for external changes shall not
> hide other important progresses" is valid. 
Thanks :-)
> 
> My code calls ProgressHandle.suspend and in spite of that the progress area
> often shows status of my ProgressHandle and not any other, not suspended
> ProgressHandles.
> 
> I guess the fix is quite simple. Rather than showing "Suspended" in the
> progress area, check whether there is another running progress and switch the
> view to it. That shall be simple change in the implementation (no API changes),
> can you make it for 6.9 Dafe?
> 
> If there is another way I can make the progress less visible
> (createSystemHandle?), feel free to recommend it and I'll use such existing
> API.
That is the second part of my concern. It would be very preferable not to use "progress bar" space for "check for external changes" activity at all. Users' hate of "Scanning in progress..." activity interpolate to any progress task now :-(
Comment 22 Vladimir Voskresensky 2010-05-07 14:15:20 UTC
I want to clarify, if it was not clear.
I do not ask to *disable* external check activity (in fact I disagree with even have such choice). 
I only ask to *hide* this implementation detail from UI visibile for usual users.

It is claimed, that external check does not concurrent with other important IO tasks and do not slow down IDE in general.
Why user have to be interested that IDE infrastructure has some internal processes? We do not create progress bar for each task sent into request processor. 

We create progress for something interesting for user. What are the conclusions user have to made when he sees progress about "checking for external changes"?

What's why I proposed to:
1) hide it at all
or 
2) if it is really needed: show in some other place, not in progress bar area. and make it as invisible as possible :-)
Comment 23 Jaroslav Tulach 2010-05-12 14:14:04 UTC
Created attachment 98858 [details]
Change to progress UI

I don't want bugzilla to be place to argue about UI spec changes. Thus I will concentrate only on divergences/obvious misbehavior from the approved UI spec in this issue.

Here is a patch with special treatment of suspended (isSleepyMode) handles. I don't claim I understand the overall code, but this at least ensures that if a suspended progress is shown, it does not replace any existing one.
Comment 24 Quality Engineering 2010-05-13 05:04:09 UTC
Integrated into 'main-golden', will be available in build *201005122200* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/
User: 
Log:
Comment 25 David Simonek 2010-05-13 14:21:55 UTC
Maybe I'm too dumb for this, but I don't understand the patch. First part - 
+            if (handle != null && handle.isInSleepMode()) {
+                initiateComponent(event);
+            }
= force show of progress bar for handles in sleep mode, even if handle is not selected...Huh?

second part:
-                event.getType() == ProgressEvent.TYPE_SILENT ||
why?

third part:
-                initiateComponent(event);
+                if (!event.getSource().isInSleepMode()) {
+                    initiateComponent(event);
+                }
yes this I can understand, don't show in bar when state changed to sleep mode

Jardo could you sum up what desired behavior should be? Thanks.
Comment 26 Jaroslav Tulach 2010-05-14 08:10:39 UTC
The desired behavior is: Rather than showing "Suspended" in the
progress area, check whether there is another running progress and switch the
view to it.

If there is a better way to achieve that, then ignore my patch.
Comment 27 Vladimir Voskresensky 2010-05-14 09:17:01 UTC
(In reply to comment #26)
> The desired behavior is: Rather than showing "Suspended" in the
> progress area, check whether there is another running progress and switch the
> view to it.
I'd like to state requirement as:
- doesn't matter in progress or suspended mode it is but 
"Check for External Changes" should have the lowest visibility priority.
Comment 28 David Simonek 2010-05-17 12:32:42 UTC
Changeset: 7359b701e78f
Author:    Dafe Simonek <dsimonek@netbeans.org>
Date:      2010-05-17 14:32
Message:   #184430: Make progresses in suspend mode less visible then non suspended progresses
Issue #184430 - "Check for External Changes" progress shall not hide other (more important) progresses
Comment 29 David Simonek 2010-05-17 12:36:32 UTC
After off line discussion with Jarda I was able to test the patch and integrate. It seems to work well in general.
Comment 30 Vladimir Voskresensky 2010-05-18 14:52:45 UTC
Created attachment 99143 [details]
incorrect labels

Now it's even worse :-( 
suspended is propagated to our tasks which definitely running
Comment 31 Vladimir Voskresensky 2010-05-18 14:53:12 UTC
we have a regression
Comment 32 David Simonek 2010-05-18 15:16:55 UTC
Strange, it works OK for me. Any exact reproducible set of steps? From the images I see that you actually clicked on progress bar - that's another story probably, but try don't clicking (=typical scenario) and you'll see suspended tasks only when no other tasks are running - at least this is how it works for me.

Btw what exactly makes it a regression?
Comment 33 Vladimir Voskresensky 2010-05-18 15:59:10 UTC
I clicked on progress bar because was confused that my task is displayed as "suspended" (although I know we never put it in such state) => that's why I've clicked to see details and there was our favorite "Check for External Changes" task in suspended mode

The mentioned regression is our tasks displayed in "suspended" mode.

I think to reproduce you need big C++ project (because we show numbers in progress bar, not only infinite activity) and then switch between IDE and other window.
I can send you this project, but it's rather big.

May be you can give me some trace flags to run with?
Comment 34 Vladimir Voskresensky 2010-05-18 16:20:38 UTC
btw, on the picture you can see that checking for external changes is still shown with higher priority than our tasks
Comment 35 Quality Engineering 2010-05-19 06:13:06 UTC
Integrated into 'main-golden', will be available in build *201005182201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/
User: 
Log:
Comment 36 David Simonek 2010-05-19 07:46:50 UTC
OK guys, I spent a night studying Progress API and impl and I think I have it. Previous patch was wrong because it broke main principle of UI part of progress - that selected task is always the one shown in progress area.
So IMHO right fix is to change selection policy and leave UI part as it was. I tested on my experimental module which I've built for this purpose and it seems to work great, so hopefully it will work for you too.
Comment 37 David Simonek 2010-05-19 07:49:16 UTC
Changeset: 31e1f5bb3cc1
Author:    Dafe Simonek <dsimonek@netbeans.org>
Date:      2010-05-19 09:49
Message:   #184430: Improve selection policy to favor non-sleepy and user initiated tasks
Issue #184430 - "Check for External Changes" progress shall not hide other (more important) progresses
Comment 38 David Simonek 2010-05-19 09:15:15 UTC
Hmm, I just found that the clone is already created :-(
Comment 39 Vladimir Voskresensky 2010-05-19 09:45:12 UTC
so, we need to port it into release69 branch... after verifying in trunk
Comment 40 David Simonek 2010-05-19 10:32:25 UTC
We're in high resistance, only show stoppers may go into branch...
Comment 41 Vladimir Voskresensky 2010-05-19 10:41:52 UTC
Ok. Let's follow process. (reopening, P1, STARTED, 69_HR_FIX_CANDIDATE)
We need this fix, because current behavior is really regression in Progress API.
We can verify you fix as soon as it appears in cnd-main.
Who can review your fix?
Comment 42 David Simonek 2010-05-19 10:48:39 UTC
Yes. I'll wait for your verification on cnd side. Jaroslav Tulach can review my fix I believe, Jardo?
Comment 43 Vladimir Voskresensky 2010-05-20 14:59:38 UTC
verified in cnd-main
Now it works great.
Thanks!
Comment 44 David Simonek 2010-05-20 15:11:40 UTC
Great, thanks.

pinging - Jardo, could you please review? Or anybody else?

Unfortunately there was merge conflict in push-to jobs, I'm working on it.
Comment 45 Stanislav Aubrecht 2010-05-21 08:59:02 UTC
i've reviewed dafe's patch and it seems to be ok.

i'd just suggest a minor improvement to ensure that always the latest progress bar get selected:

# This patch file was generated by NetBeans IDE
# Following Index: paths are relative to: D:\hg\core-main\api.progress\src\org\netbeans\modules\progress\spi
# This patch can be applied using context Tools: Patch action on respective folder.
# It uses platform neutral UTF-8 encoding and \n newlines.
# Above lines and this line are ignored by the patching process.
Index: TaskModel.java
--- TaskModel.java Base (BASE)
+++ TaskModel.java Locally Modified (Based On LOCAL)
@@ -96,7 +96,7 @@
 
         // select last added that is not in sleep mode and preferrably userInitiated
         InternalHandle toSelect = null;
-        for (int i = 0; i < model.size(); i++) {
+        for (int i = model.size()-1; i >= 0; i--) {
             InternalHandle curHandle = (InternalHandle)model.getElementAt(i);
             if (getSelectionRating(curHandle) >= getSelectionRating(toSelect)) {
                 toSelect = curHandle;
Comment 46 Jaroslav Tulach 2010-05-24 11:13:28 UTC
As far as I can say the 31e1f5bb3cc1 changeset is much better than my previous attempt to address this issue. The introduction of the updateSelection method is more systematic. It concentrates the selection logic in one place. As far as I can say the change looks OK.
Comment 47 madhurtanwani 2010-06-01 07:15:05 UTC
Is this fix available in the 6.9 dev release? or 6.9 RC?
Comment 48 David Simonek 2010-06-01 07:37:13 UTC
Yes, it is:
http://hg.netbeans.org/release69/rev/1ba9680101b5


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo