Community
Participate
Working Groups
Command framework supports deferred for href callback so natural would be for visibleWhen to support a deferred as well. Until now we didn't need it but now I really need it for Bug 348600.
Created attachment 197888 [details] proposed fix This is a simple fix that checks if value returned by visibleWhen is a deferred. The code that rendered the command after checking result of visibleWhen was moved to a separate function that is called on "then". I've tested it with currently existing commands and it doesn't break anything. It works as well with a fix for Bug 348600 I made that returns a deferred.
Gosia could you commit this to a remote branch so I could review this and try it out.
I'm not really happy with this approach, I wouldn't want to step up to support this in general in the command framework. I know we have deferred support for hrefs and command callbacks, but "visibleWhen" (and future "enabledWhen") seem very different to me. These are things that can get called at menu open time, etc., or when we decide to recompute page state, and so far we have specifically avoided asynchronous calls here. This is why we built the "validationProperties" info in fileCommands, so that validation info could be given in advance by a plug-in without having to call out to the plug-in to get it. I'm also reminded of bug 343839. Where we got burned because there were server requests being made to provide UI information. In general I think we should only really support deferred for either the run method (doing the work) or else the href (since it's kind of like run.) So far in the command framework we only use deferreds to fill in information in a dom element or dojo menu item that has already been created and placed. So we might fill in an href or text label after a deferred, but we don't actually add the dom item in a deferred. This makes me nervous. I would rather save asynch requests for calling out to do work or filling in attributes of existing dom elements (such as href). Is there any better approach for fixing the triggering bug? If this is really the only way to deal with the problem, I would consider this a temporary hack and we would come up with a better way post 0.2. I don't have a good read as to whether this is a realistic use case for other plug-ins or a sign of something missing in the server API.
I talked to Szymon and we came out with an idea that we could modify API to return information. This would allow to provide the same functionality without visibleWhen change, it's tracked in bug 349289. I agree that doing extra calls should be avoided. But I don't see much difference between visibleWhen and href callback. For the href callback we do the extra call while rendering the command, not when user clicks it. Because I think that fixing bug 349289 would be a better solution anyway, so I'm fine with closing this bug for now. But I think it's not a bad idea to come back to this discussion in the next release. We'll probably have some more cases to implement then and some more time for discussion and tests. Simon, if you still want to review and test the changes they are committed to branch Bug349195 on dev.eclipse.org
After fixing bug 349289 I have everything I need, so I don't need this fix any more. I propose to get back to this discussion in next release.
(In reply to comment #5) > After fixing bug 349289 I have everything I need, so I don't need this fix any > more. I propose to get back to this discussion in next release. There is some mention of this in bug 341540. Szymon was asking for the deferred href feature and it happened to already work, but we have never thought through this case and I don't much like it. It causes performance problems as you mention. (I think it's still different/not as bad as visibleWhen because at least the dom notes are created synchronously and then the info is filled in asynch). I'll cc: you on that bug and write more there.
