Community
Participate
Working Groups
Build ID: I20090611-1540 Steps To Reproduce: Currently there is no support for checkbox for the flexible hierachy debug view. More information:
Created attachment 144227 [details] Patch for checkbox support
Created attachment 144229 [details] Test view for the patch I reused the midi example to show the checkbox support for the MidiEvent object. The initial check state is set to true for all nodes, the check event is printed in the console.
Created attachment 144845 [details] Updated patch for check support. In general I think it's a good extension of the viewer. I only added some checks, comments, and renamed some things. I imagined that this feature would be more complicated, but piggy-backing it off label provider is a very good idea since usually the check box will need to be updated at the same time as the label. Also, this allows the model to use the IModelDelta.STATE flag to notify the viewer that the check box needs to be updated. I wasn't able to use the example because I'm running on linux and the midi thing doesn't work for me. So I'll try to put together another test. Darin, do you have any objections to committing feature? There's 102 lines of code in Patrick's patch so I don't think we need a legal review.
Assigning to myself for review.
Hi Darian, I am wondering if you get a chance to review this patch yet, is there any additional adjustment required for this patch? Regards, Patrick
Sorry, haven't had a chance to review this yet. It is on my list.
When trying out the sample, the check boxes can be toggled. I don't think this should actually work for the sample as the getChecked method of the label provider returns just true, regardless of any state. Also the IElementCheckReceiver/IElementCheckListener does not modify the state, it just prints to the console. I would think that the view should only change the checked state once the model did change, or if the view does change right away it should at least update the checked state once by asking the label provider for an updated state.
Created attachment 146453 [details] Modified setChecked to return a boolean if setChecked returns false, than the checkbox state is reverted back to the old state.
Created attachment 146454 [details] Updated test plugin
(In reply to comment #8) > Created an attachment (id=146453) [details] > Modified setChecked to return a boolean > > if setChecked returns false, than the checkbox state is reverted back to the > old state. We probably should do this as well when there is no IElementCheckReceiver, which actually was my initial setup. Also I wonder if we should ask the label provider for an update too, in the end the view changes its visible state for a change which does not come from the label/content provider, so I would think we should make sure the new checked state is really the new state of the model.
Created attachment 146458 [details] Modified setChecked to return a boolean Daniel, Give this a try, it now handles the case of no IElementCheckReceiver. I don't think and additional update is required, the state of the checkbox is reverted back to the original state if the setCheck returns false or there is no IElementCheckReciver. Patrick
Patrick, I could not get the latest patch to apply without error against HEAD. The first two patches apply, but I'm not sure that's what I should be reviewing?
(In reply to comment #12) > Patrick, I could not get the latest patch to apply without error against HEAD. > The first two patches apply, but I'm not sure that's what I should be > reviewing? Darin, I updated to the latest HEAD, and I have 4 plugins in my workspace, debug.core, debug.ui, debug.example.core, and debug.example.ui. I am able to compile without error. Can you let me know what error you are getting?
Ah, now I get it... Apply the two most recent patches selectively against examples and debug.ui. It's working now.
Thanks Patrick. The patch looks good. I have one suggestion that we should consider. To perform check callback notification, this implementation requires clients to register an IElementCheckReceiver per element. Up to now, interactions between the model and the viewer have been handled by the IModelProxy. I'm wondering if it would make more sense to notify an element's model proxy for check state callbacks. This would centralize model/viewer interactions in one place and possibly reduce the number of adapters that need to be registered. The trick would be to identify which model proxy belongs to an element when it gets checked (via element tree path prefixes, I suppose). This would require an optional extension to an IModelProxy as well - i.e. ICheckboxModelProxy or something like that. The only other comment I have is that the two new methods in ElementLabelProvider (getChecked(...) and getGrayed(...)) should probably have protected visibility rather than public (since they are intended to be overridden by subclasses, rather than called as API).
Another question - what's the intended way for a model to change the check state in a viewer? Should it fire a IModelDelta.STATE change for elements? Is it up to the model to decide what happens to the check state of parents when a child is checked. Usually, logic is present to update the parent check state when a child is checked/unchecked (i.e. to show whole or partial selection states).
(In reply to comment #16) > Another question - what's the intended way for a model to change the check > state in a viewer? Should it fire a IModelDelta.STATE change for elements? Yes. > Is it up to the model to decide what happens to the check state of parents when > a child is checked. Usually, logic is present to update the parent check state > when a child is checked/unchecked (i.e. to show whole or partial selection > states). I don't want to handle the state of the parents in the viewer, I don't think the viewer should assumes what the model wants, i.e have it's parents checked or partially checked. Instead, let the model decide it's parent states.
(In reply to comment #17) > I don't want to handle the state of the parents in the viewer, I don't think > the viewer should assumes what the model wants, i.e have it's parents checked > or partially checked. Instead, let the model decide it's parent states. For stand alone models, this makes sense. For mixed models (i.e. contents from different models), there might be issues. I have not looked at the breakpoints view yet... but I assume the view will have to take care of some of the check state logic since a breakpoint won't know about all its parents. For example, breakpoints don't know about groups or the other breakpoints in a group.
Patrick, will you be able to address comment #15 by end of week? If not we should push to M3. Next week is M2, so we'd need the fix in by Monday (Sep 14th), latest.
As far as I now Patrick is out on vacation this week.
(In reply to comment #20) > As far as I now Patrick is out on vacation this week. Patrick is on vacation this week. He will be back on monday (14th). It is probably best to target this for M3.
OK, moving to M3.
Created attachment 147214 [details] Check Event notification on the model proxy Darin, This patch contains check event notification on the element model proxy, as well as making the two methods in the ElementLabelProvider to protected. I'll attach the example code shortly. Patrick
Created attachment 147216 [details] Example for listening to event notification on the model proxy Example code for listening to event notification on the model proxy.
Thanks Patrick - the updates look good. Will apply once M2 is declared with some minor javadoc updates.
Applied to HEAD. Fixed. Please verify, Pawel.
I noticed that the ICheckboxModelProxy is notified in the UI thread... Depending on what effect this has in the model, the notification could be done asynch, in a non-UI thread. Thoughts?
(In reply to comment #27) > I noticed that the ICheckboxModelProxy is notified in the UI thread... > Depending on what effect this has in the model, the notification could be done > asynch, in a non-UI thread. Thoughts? I think having the notification on the UI thread has a better visual effect, when client isn't able to handle the setChecked notification. This required reverting the check box state in the widget back to the existing state.
(In reply to comment #28) > I think having the notification on the UI thread has a better visual effect, > when client isn't able to handle the setChecked notification. This required > reverting the check box state in the widget back to the existing state. True... however, the effect of the check could result in communication with the model (for example, enabling a breakpoint in JDT results in sending requests to the target, etc). Perhaps we need to spec that if the check requires model communication, that updates should take place asynchronously. If the udpate fails, the model needs to send a delta to refresh the view such that the check state gets properly updated (reflects the underlying model). So, the check box beomes a trigger - but the end result may take time.
(In reply to comment #29) For request that takes a long time to process, it is probablly best to do what you suggested. Do you want me to file a new bug? or modify the existing patch?
(In reply to comment #30) > (In reply to comment #29) > For request that takes a long time to process, it is probablly best to do what > you suggested. Do you want me to file a new bug? or modify the existing patch? The code is in HEAD. I just updated the javadoc comment: * <p> * This method is called in the UI thread. Clients that execute long * running operations or communicate with a potentially unreliable * or blocking model should run those operations asynchronously. * </p>
Looks good to me. I also enabled the unit tests for the check box support in the new test plugin. But I was not able to create a unit test for the check received because I don't know how to trigger a SWT.CHECK event without user interaction.
