Bugzilla – Full Text Bug Listing
| Summary: | [StatusHandling] WorkbenchStatusDialogManager should inform of dialog closure | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Szymon Brandys <Szymon.Brandys> | ||||||||||||||||||||
| Component: | UI | Assignee: | Krzysztof Daniel <krzysztof.daniel> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||||
| Priority: | P2 | CC: | Kevin_McGuire, krzysztof.daniel | ||||||||||||||||||||
| Version: | 3.4 | ||||||||||||||||||||||
| Target Milestone: | 3.5 M3 | ||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||
| Whiteboard: | hasPatch | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Szymon Brandys
Changed summary from: Add listeners mechanism to WorkbenchStatusDialogManager Created attachment 113730 [details]
Work in progress
I'd like to show the direction of work.
Changes are:
* listener in WSDM launched when closing is not a result of modality switch
* getShell made private again
* modified blocking UI loop in WEH - it is not necessary to rely on shell.
* I'll probably open a bug against "Thread.yield" in the blocking loop, because it is possible that Display.sleep is more accurate solution (not sure yet, I have to check/consult).
* basic test
I have assumed that those changes should not be a part of API, so listener will be moved to the internal package. Created attachment 113856 [details]
Fix with tests
Created attachment 113857 [details]
mylyn/context/zip
Created attachment 113964 [details]
Updated Fix
During discussion with Szymon we decided to move the listener to the API package but mark is as experimental.
How about using IPropertyChangeListener or maybe another existing mechanism for notifying about dialog changes? We do not have properties in WSDM, so using this mechanism would be a hack. Listening to dialogs is not supported by SWT/JFace. On the other hand there is Shell Listening mechanism, but again, it would be a hack as we should ignore certain shell close events (modality switch). I think that if we want to this right we should: * open an RFE against SWT/JFace to support dialog state changes. * use the mechanism introduced in previous point From another point of view it is rather unreasonable to engage powerful tools to achieve such a small functionality, therefore I'd rather imagine releasing last patch as it is now, and, depending on the JFace team decision on RFE, stabilizing it or migrating to new dialog listener mechanism. Sounds reasonable? (In reply to comment #8) > We do not have properties in WSDM, so using this mechanism would be a hack. > Listening to dialogs is not supported by SWT/JFace. What do you mean? What is the problem with adding WSDM#addPropertyChangeListener? I was thinking about defining DIALOG_STATE property for WSDM, not the dialog. > From another point of view it is rather unreasonable to engage powerful tools > to achieve such a small functionality. Hmm... powerful? It's just an interface to implement, right? DIALOG_STATE property requires full dialog state management (OPENED, CLOSED, REOPENED). We need only CLOSED one and I do not want to add extra functionality until it is requested. Implementing this listener partially is not an option, because then we contradict the idea of property. The thing that really concerns me is bug 220557 which may need another events (deferred statuses passed, deferred statuses handled which equals transition "CLOSED->OPENED"), and I'd like to be sure that we support only minimal set of events, because someone has to maintain it. To sum up: I think that IPropertyChange support exceeds the scope of this bug and it is to early to introduce it. The best moment for me is after 220557 is fixed and we have rather clear vision of what we need to listen to. (In reply to comment #10) > DIALOG_STATE property requires full dialog state management (OPENED, CLOSED, > REOPENED). I think that OPENED, CLOSED would be enough. > To sum up: I think that IPropertyChange support exceeds the scope of this bug > and it is to early to introduce it. The best moment for me is after 220557 is > fixed and we have rather clear vision of what we need to listen to. What I am trying to say is that we should add well-thought API. As I understand you are going to extend API anyway to fix some other issues, so IPropertyChange API sounds more reasonable... but I may be wrong ;-) Created attachment 114310 [details]
Fix with property change support
I hope it is better now
Created attachment 114311 [details]
mylyn/context/zip
Created attachment 114933 [details]
Szymon's proposal
Why does the patch contains testBlockingBehavior2/1 tests? Are they related to the issue?
I think that we maybe could have DIALOG_SHELL property instead of DIALOG_OPENED. And this property would have new shell set as the newValue and old one for the old Value.
Thoughts?
Test are connected with this bug - we modify the blocking mechanism and I want to be sure that it works correctly with IPropertyChangeSupport. Boolean.TRUE/FALSE is a very clean solution, but I do not like the limitation of 2 states (which may be not enough/difficult to extend if we would like to open the WSDM for subclassing). Kevin, what do you think? I obtained some info from UI team. Paul W. suggested to use IShellProvider interface, what seems to be a good idea :-) Created attachment 115700 [details]
Fix with IShellProvider
Created attachment 115714 [details]
Fix with IShellProvider (another proposal)
Released to HEAD. |