Re: [DPP-Devel] UI: BrowserManager

[Matthias B. <mat...@gm...>]

I forget to send to all, when answering. Sorry.

Tl'tr

While answering I racked my head about one problem: If a dialog have a fix
browser and a browser have a fix page, for the whole lifetime - why not let
the browser use the renderers directly without using the BrowserPage
construct. As we want to get rid of the BrowserPage has Browserfunctions
relationship [1], I believe we could get rid of the BrowserPage classes all
together. This class only holding the path to the html file (which isn't
more than a String in the end).

I don't know if Christian still reading this mailinglist, but was there a
problem in IntelliJ that actualy made the replacement of the browser in a
shell necessary? I have vague remembrance, that minimizing the view forces
a browser replacement? But I could be very wrong here...

[1]
http://saros-build.imp.fu-berlin.de/gerrit/#/c/3033/1/de.fu_berlin.inf.dpp.ui/src/de/fu_berlin/inf/dpp/ui/ide_embedding/BrowserCreator.java


2016-04-08 14:28 GMT+02:00 Matthias Bohnstedt <mat...@gm...>
:

>
>
> 2016-04-07 19:09 GMT+02:00 Zieris, Franz <Fra...@fu...>:
>
>> Hi there,
>>
>> I started thinking about a few more test cases for the UI project
>> and I came along the BrowserManager [1].
>>
>> I don't fully understand the idea of the "setBrowser" method.
>> It says that it "sets or replaces there browser for the given page
>> and all corresponding renderers".
>> Is there a reason for the "replace" part?
>>
>
> I don't think that the browser is really "replaced" literal sense of the
> word at any time. If a dialog shell or a view with a html page is shown,
> the browser is never replaced during the lifetime of that shell. Even if
> the BrowserManager says that "the browser instance for one window may
> change, this class allows the replacement of each browser"[1], we don't do
> this at all. And I don't see any reason to do so in the future. It's a
> relict from a very early stage of the HTML UI. The only time a new browser
> is created (and setBrowser is called), is when a new shell (aka a dialog or
> a view) is created.
>
>
>> I guess these paths will be called multiple times during a plugins
>> lifetime. Every time "createBrowser" is called, a new Browser object
>> is created and passed to the BrowserManager.
>>
>>
> Yes this is the case.
>
> Now consider the hypothetical calls (e.g. in a test case):
>> > mgr.setBrowser(mainPage, browserOne);
>> > mgr.setBrowser(mainPage, browserTwo);
>>
>>
> This is indeed just hypothetical, because the mainPage should never be
> displayed in another view than the SarosViewBrowserVersion. While you
> *could* open a Dialog with the mainPage, by calling
> DialogManager.showDialogWindow(MainPage), which would lead to a change of
> the mapping as you mentioned it, you shouldn't. And I don't see any use
> case to do so. And therefore no such test case.
>
> Ever other dialog (with it's browser page and the corresponding browser
> instance) is registered in the DialogManager. You shouldn't been able to
> open the same BrowserPage multiple times,  as the dialog manager won't call
> BrowserCreator.createBrowser() -->.setBrowser()  in this case.[2] The
> MainPage is the only page that is not registered in the DialogManager.
>
> If you look at the source code of BrowserManager.setBrowser, you'll see
>> that the old browserOne will indeed be replaced in the page-to-browser
>> mapping by the new browserTwo.
>> But what about the renderers?
>> According to the source code, each of the two renderers that the MainPage
>> uses (stateRenderer and accountRenderer) now has *both* browserOne and
>> browserTwo in its "browserList". This is because "setBrowser" only
>> calls "renderer.addBrowser()" but not "removeBrowser" -- which is called
>> every time a Browser is disposed.
>>
>> This is because the only time a browser actualy needs to be removed is
> when the shell (with the browser) is disposed.
>
>
>> So, again my question: What about the "replace" part of the "setBrowser"
>> method? It sounds like the old browser object (browserOne in the example
>> above) is "relieved of its duty", but until it is disposed, it will
>> still provided with the events generated by the renderers.
>>
>
> Again: There is no actual replacement of a browser during the lifetime of
> its containing shell. So the disposing and 'relieve of its duty' should
> happen at the same time.
>
>
>> Or in other words: What would be valid test cases?
>> Is the example above simply a non-correct usage of the BrowserManager?
>>
>
> I don't think so. This is not the way these method is used. (At least it
> shouldn't be).
>
> The question I got while I looked thought this classes is: Why we have 2
> mappings that seems to manage kind of the same thing and what's there
> correlation?
>
> BrowserManager:
> Map<Class<? extends IBrowserPage>, IJQueryBrowser> Page:Browser
>
> DialogManager[3]
> Map<String, IBrowserDialog> openDialogs String (=the path of the html
> file) : Dialog
>
> Since the BrowserPage and the the BrowserPage.getRelativePath() are
> semantically even, we got 2 mappings Page:Browser and Page:Dialog. Because
> a browser dies and lives with it's dialog (wrapping shell), wouldn't it
> make sense to have a single Browser:Dialog mapping instead. The browser
> already contain the page it is displaying (handed over on it's creation),
> and this page is never changed during the browser lifetime.
>
> [1]
> https://github.com/saros-project/saros/blob/master/de.fu_berlin.inf.dpp.ui/src/de/fu_berlin/inf/dpp/ui/manager/BrowserManager.java#L12
> [2]
> https://github.com/saros-project/saros/blob/master/de.fu_berlin.inf.dpp.ui/src/de/fu_berlin/inf/dpp/ui/ide_embedding/DialogManager.java#L49
> [3]
> https://github.com/saros-project/saros/blob/master/de.fu_berlin.inf.dpp.ui/src/de/fu_berlin/inf/dpp/ui/ide_embedding/DialogManager.java#L24
>
> Cheers,
> Matthias
>