Bug 400356

Comment 1 Miles Parker 2013-02-08 13:39:13 EST

1. Modify model in order to provide details that are provided from Gerrit utility and used in editor but aren't currently supported in model.
2. Implement very simple Remote Manager.
3. Factor out much of the GerritUtils into a Remote Manager.

The remote manager will provide some basic life-cycle and notification functionality.

Comment 2 Steffen Pingel 2013-02-08 15:30:39 EST

+1 that sounds good. We may just need a simple marshaling service initially along the lines of what is proposed in https://git.eclipse.org/r/#/c/10247/.

Comment 3 Miles Parker 2013-02-13 17:48:31 EST

Created attachment 227050 [details]
mylyn/context/zip

Comment 4 Miles Parker 2013-02-28 14:50:07 EST

Okay folks, https://git.eclipse.org/r/#/c/10247/ is perhaps getting a little ambitious now. ;) The issue is that as I unravel the stuff into a remote API it seemed appropriate to break out all of the patch set buttons into remote UI factory. But there are some real opportunities to combine / coordinate that API with the existing GerritOperation stuff. All of this will implicate the design as we move ahead.

I still need to get the update stuff working correctly. But when we finish this we should be pretty close to finished the parent bug 400168 as well and a lot of bug 400167. Considering moving the review to that task, actually.

Note one significant functionality change. Since everything is loaded "lazily" (even thoough it is in fact requested as soon as the user opens the editor) the contents of Review and Patch Set sections are empty for a short period when the editor is opened. Once we have common storage (Bug 394020 bug 389944) that will be less of an issue.

I think given the scope of changes this may be a case where we should consider a comprehensive code review / discussion for this and some follow-on review commits with a relativly light-weight initial review. My reasoning is that a lot of this code will evolve as we implement handling for shared review resources -- something we'll need to resolve bug 399700 among others. That in turn is dependent on bug 400165, because I think we'll want support for transational editing domains to support decoupling from specific editor implementations. Steffen, given size of code changes here, and how interrelated they all are, how would you prefer to handle this?

Comment 5 Steffen Pingel 2013-03-02 08:20:38 EST

The scope of the change is far beyond this bug report. You'll have to break it down before we can consume it. The remote API abstraction can be implemented without changing all of the editor implementation.

I'm a bit confused about the introduction of AbstractPatchSetUiFactory. I thought we had already agreed that a modal dialog workflow isn't the way to move forward. Why are we creating a new substantial framework and abstraction to support that instead of keeping it simple?

Comment 6 Miles Parker 2013-03-04 12:42:36 EST

(In reply to comment #5)
> The scope of the change is far beyond this bug report.

Agreed.

> You'll have to break it down before we can consume it.

Let's discuss how best to do this w/o getting into rebase hell. Sebastien and I have discussed some ideas on how to proceed with these much larger chunked commits. We'd like to see about doing an R4E review, with the thought of hosting the review artifacts themselves on github or somewhere else so as to satisfy need for transparancy. Not ideal, but should be workable until we have a Gerrit based R4E repository.

> The remote API abstraction can be implemented without changing all of the editor implementation.

Yes, but the editor implementation could not usefully be factored out without implementing the Remote API. :) I've been using the editor implementation to guide the development of the Remote API, and this exploratory approach has allowed me to understand key concerns in a way that trying to develop the perfect Remote API in isolation would not have. Yes, now that we have (almost) done that, it may make sense to turn things around and look at the Remote API and other generic stuff first.

Note that UI became unavoidably mixed up in this precisely because UI is currently tied up in the page logic. So part of this effort has been to isolate the UI bits as well...

> I'm a bit confused about the introduction of AbstractPatchSetUiFactory. I
> thought we had already agreed that a modal dialog workflow isn't the way to move
> forward. Why are we creating a new substantial framework and abstraction to
> support that instead of keeping it simple?

For me, simple means well-structured and isolated -- and those long sections of boiler-plate code in the editor were hard for me to follow. So I don't look at it as a substantial framework; just a better way to isolate the concerns here -- and probably temporary. By moving them out of the page editor code, the overall structure and relationships become much clearer. (e.g. the parallel Dialog, Operation, etc.. code)  So I've been able to get rid of most of the editor/push behavior coupling and now we can much more easily focus on turning that into a more non-modal / submit oriented workflow. Note that the remote API will need to support both push and pull -- right now we're just doing pull, and I need to begin to design how to isolate the UI components from the push side as well, which is a trickier and more interesting problem :) and one for which these changes make things much more obvious.

Note that I've also been working on making the Remote API agnostic about where it is used. You can create a single Factory/Consumer pair for an editor, or use a factory for the entire workspace w/ multiple consumers per view/editor or anything in between. This will allow us to easily play with workbench wide support. That in turn will make solving bug 399700 quite straightforward.

All of this supports the goal of isolating the editor from model and workbench concerns it doesn't need to have.

Comment 7 Miles Parker 2013-04-18 18:00:06 EDT

Merged: https://git.eclipse.org/r/#/c/11012/