Created attachment 127858 [details]
Proposed patch

MK1: is the property intended to be used only on startup or is it meant to be (re)set while the IDE is running?

Shouldn't toRefresh be WeakSet?

Re. MK01. This property is intended to be changed during runtime. As there are no notifications about changed properties, the code that checks the property does not look very nice. Tomáš Zezula even mentioned that he'd like to avoid busy waiting in RepositoryManager (when it is suspended by this property). We will need some API enhancement for that.

Re. "toRefresh" - so far the set was short living, but now, when it can grow for minutes, it is probably safer to make it a weakset.

TZ01: The current patch requires clients of the "API" to do active spinning with some possible hand out.
This can become quite inefficient. If other processes are ON_PROC there are uselessly preempted by the spinning thread. Also the spinning thread prevents kernel memory manager to swap out the process pages. The API should provide notification, best it will be a service with await() method or java.util.concurrent.Future<?>. If no API should be introduce I suggest a special logger logging {suspend, resume} messages which can be handled by a Handler to unschedule the thread, but this is just a workaround as Logger should be used just for logging, the API will be much better.

To _ tboudreau: In current design the toRefresh, pending can be weak, the FileObjects used by recursive listeners are retained by FileObjetKeeper and for other cases the events from GCed FO are probably not important.

(In reply to comment #4)
> Re. MK01. This property is intended to be changed during runtime. As there are
> no notifications about changed properties, the code that checks the property
> does not look very nice. Tomáš Zezula even mentioned that he'd like to avoid
> busy waiting in RepositoryManager (when it is suspended by this property). We
> will need some API enhancement for that.
> 

Well does it expect exactly just one client? if not, we might get a problem there. If client A sets it to true, then client B sets it to true, both run concurrently but A is significantly shorter than B, then A is setting it to false, but B still expects it to be true for some time.

Created attachment 128121 [details]
Improved patch

TZ01 is addressed by the recommendation to use notifyAll().

MK01 arch.xml explicitly talks about changing the property and suggests to do it incrementally now.

Per recommendation I've made the set weak. It won't help with recursive listeners, but it might be useful for folders with normal flat listeners.

-1 from me on this API proposal. A property is OK to switch something on or off upon IDE startup. But turning a property into an API for multiple clients as described in the arch.xml is, IMO, an ugly hack. Btw. you never mentioned who is the client of this API, that is who and when will set/unset the property.

According to the API review guidelines, every reviewer can turn a fast track review into a standard review. Unless that happens I plan to integrate this change on Nov 22, 2012.

DK01 - who is the client of this API, that is who and when will set/unset the property?

DK02 - why don't you just propose a regular Java API? Especially when you are encouraging other modules to use this. What's so good, in your opinion Jarda, in writing all this code:

  // first "API" call: set suspend state:
  final String prop = "org.netbeans.io.suspend".intern();
  synchronized (prop) {
      int prev = Integer.getInteger(prop, 0);
      System.setProperty(prop, "" + (prev + 1));
      prop.notifyAll();
  }

  // second "API" call: wait until suspended state is lifted:
  for (;;) {
    int cnt = Integer.getInteger("org.netbeans.io.suspend", 0); // NOI18N
    if (cnt <= 0) {
        break;
    }
    try {
       final String prop = "org.netbeans.io.suspend".intern();
        prop.wait();
    } catch (InterruptedException ex) {
        LOG.log(Level.FINE, null, ex);
    }
  }

instead of just

  // first API call: set suspend state:
  someAPI.suspend();

  // second API call: wait until suspended state is lifted:
  someAPI.waitWhileSuspended();

DK03 - I do not think hacks like this belong to the platform unless there is a strong justification. Is there one?

DK01 - the intention of this API is to allow creation of a UI that would allow the user to be "in control" over interaction of his IDE and his command line tools like git or hg. The UI is however not subject of this issue.

DK02 - I don't feel able to commit to stable API right now. The non-Java API allows me to expose the functionality and give it different stability. If the API proves useful, we can always wrap it in a classical Java API later (as we did with FileObject.revert, for example).

Still on track to integrate on Nov 22, 2012.

(In reply to comment #12)
> Still on track to integrate on Nov 22, 2012.

I'm sorry Jarda, but your insistence is forcing me to remove API_REVIEW_FAST. I do not understand your rush with this issue. Why??

re. DK01 - if there is no client of this API in NetBeans 7.3 then there is no reason to rush this API enhancement into NetBeans 7.3 either. Issue 194147 caused quite a bit of controversy and I have not seen yet any internal nor external agreement on how it should/could be resolved. In that light, implementing an API enhancement for something what have not yet been agreed does not make sense. We are also two weeks from the start of FCS period.

re. DK02 - perhaps platform should have an "incubator" module with Friend level API contract where a new API like this one could mature. Or are you saying that any new APIs in the platform must be always stable APIs? That must be quite limiting, no?

David, you have right to request standard API_REVIEW. 

I believe we have all the materials (e.g. the patch with arch.xml changes) for the review ready. The next step is to select reviewers and call a review meeting. I'd like Petr Somol, Tomáš Hůrka and Tomáš Zezula to be voting reviewers. The 4th vote is yours. 

I am not sure what time would work for you, when you are on the other side of the world, but Prague could have a review meeting on Monday 10am. Is it OK?

Let's meet once we have a client of this API and that client is part of a NetBeans release. Without the client there is nothing to discuss. And once we have the client this issue should be straightforward.

API change without a written down client is not common, but it is not against any rules. You can join the review meeting to get an explanation.

Your opinion has received fair treatment so far. If you are unwilling to defend it, there is no reason to assign it any technical value. Such contribution is useless in an API review process. However, if you change your mind, me and other reviewers will be glad to hear you on Monday Nov 19, 10am CET.

> Such contribution is useless in an API review process. 

I strongly disagree. Any contribution that strives to simplify or remove unnecessary API (such as comment #9) is very valuable. 

Seriously, talking about this API without discussing the client(s) and overall use cases is a joke.

> If you are unwilling to defend it...

It's the other way around: the person who is proposing the API must justify and defend why the API is needed.

(In reply to comment #16)
> Your opinion has received fair treatment so far. If you are unwilling to defend
> it, there is no reason to assign it any technical value. Such contribution is
> useless in an API review process.

What exactly am I supposed to be defending??? This is turning into another parody just like issue 194147. I do not have time for this game Jarda and I'm not enjoying it either. You asked API reviewers for a feedback on your proposed API and I gave you one. If you do not like it that's your problem. If you do not want to use it that's your decision.

You are the one who has always been advocating and pushing usecase driven API design. And I absolutely agree with you on that. It made a huge positive impact on quality of most of the new NetBeans APIs. Why do you want to twist the rules in this issue without providing *any* reasoning is beyond my understanding.

> However, if you change your mind, me and
> other reviewers will be glad to hear you on Monday Nov 19, 10am CET.

No, I'm not joining. I've said everything in this issue.

The review meeting is over. All present reviewers approved the API for integration as soon as 7.3 is branched under the condition that it will be classified with "under development" stability.

As a poor man's solution to issue 194147 the reviewers agreed with a "pause the IDE" module on a contrib using this API.

The reviewers moreover stressed the need to really focus on the IDE+VCS interaction while planning the next release and address the feature requests expressed in issue 194147 either by detecting "back in time" repository changes,  improving IDE's UI to support the "backport a fix" use-case, and/or providing better support for working with Hg queues - all to eliminate needless scanning.

ergonomics#c61d21c3ce10

Integrated into 'main-golden', will be available in build *201301150001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/c61d21c3ce10
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #222193: Property based API to suspend reaction to changes comming from native listeners

This API has been used in http://wiki.netbeans.org/MasterFileSystemSuspend