review of commit ea30719

Joseph Marques jmarques at redhat.com
Mon Dec 13 21:44:47 UTC 2010


On 12/13/2010 04:25 PM, Ian Springer wrote:
> On 12/13/2010 01:41 PM, Joseph Marques wrote:
>> On 12/13/2010 01:21 PM, Ian Springer wrote:
>>> On 12/13/2010 12:47 PM, Joseph Marques wrote:
>>>>> commit ea307199f414e3f155c4ababaabb5fee769c6c9c
>>>>> Author: Ian Springer<ian.springer at redhat.com>
>>>>> Date:   Mon Dec 13 12:30:56 2010 -0500
>>>>>
>>>>>          2nd attempt at fixing subtab refresh when switching back to a subtab from another subtab on the same Resource or group, hopefully this time without breaking TableSection detail views (https://bugzilla.redhat.com/show_bug.cgi?id=661528); disable ack-all and delete-all buttons on alert history view if there are 0 alerts in the history
>>>>>
>>>> ...
>>>>> modules/enterprise/gui/coregui/src/main/java/org/rhq/enterprise/gui/coregui/client/inventory/common/detai/AbstractTwoLevelTabSetView.java
>>>>> @@ -240,11 +240,12 @@ public abstract class AbstractTwoLevelTabSetView<T, U extends Layout>      extends Lo
>>>>>                   if (subView instanceof BookmarkableView) {
>>>>>                       // Handle any remaining view items (e.g. id of a selected item in a subtab that contains a Master-Details view).
>>>>>                       ((BookmarkableView) subView).renderView(viewPath);
>>>>> -            } else if (subView instanceof RefreshableView) {
>>>>> -                if (subView.isDrawn()) {
>>>>> -                    // Refresh the data on the subtab, so it's not stale.
>>>>> -                    ((RefreshableView) subView).refresh();
>>>>> -                }
>>>>> +            }
>>>>> +
>>>>> +            if (subView instanceof RefreshableView&&      viewPath.isEnd()&&      subView.isDrawn()) {
>>>>> +                // Refresh the data on the subtab, so it's not stale.
>>>>> +                Log.debug("Refreshing data for [" + subView.getClass().getName() + "]...");
>>>>> +                ((RefreshableView) subView).refresh();
>>>>>                   }
>>>>>
>>>>>                   this.tabSet.markForRedraw();
>>>> Now I think we're dealing with a race condition.  Since many
>>>> RefreshableViews are also BookmarkableViews, both checks may catch.
>>>> However, the renderView(ViewPath) method is not guaranteed to do all of
>>>> its work synchronously, so we may be calling refresh() before some or
>>>> all of the asynchronous work inside renderView(ViewPath) completes.
>>> Ugh, yes, you're right. In fact, this is how the code used to be, and
>>> Jay changed it at some point to prevent that race condition. Since our
>>> renderView() API currently provides no viewRendered() callback
>>> mechanism, I think the easiest way to address this for now is for a view
>>> V that implements both BookmarkableView and RefreshableView (the case
>>> I'm trying to cover here), make V's renderView impl responsible for
>>> calling refresh() on itself when it's all done doing its own thing. What
>>> do you think?
>> If this were a rare scenario, I'd agree that your suggested workaround
>> is sufficient.  In this case, however, the set of classes that implement
>> RefreshableView has a high degree of overlap between the set that
>> implement BookmarkableView, so it might just lead to more bugs from
>> inconsistencies due to lack of enforcement.  So I'm inclined to believe
>> it's going to save us more time and headache in the long run if a
>> solution is implemented at the infrastructure layer, not the individual
>> views.
>> _______________________________________________
> It actually is relatively rare. The change to renderView() would only
> need to be made in the following classes:
>
> - Table
> - (resource)ActivityView
> - (group)ActivityView
> - ResourceResourceAgentView
Would it have to be done in all direct implementers of RefreshableView?  
If so, then we're missing the resource/group views for plugin/resource 
configuration.
> If we did it at the infrastructure level, I guess we could renderView to
> make a callback to CoreGUI upon fully rendering its view:
>
> CoreGUI.viewRendered(BookmarkableView renderedView)
>
> and then viewRendered() would do:
>
> if (renderedView instanceof RefreshableView) {
>      ((RefreshableView)renderedView).refresh();
> }
>
> but this would require every single impl of renderView to be updated to
> make the callback, so it would still involve impls to be updated, and
> even more of them. I don't think the framework is centralized enough to
> make changes only at the infrastructure level.
The above suggestion isn't much different from the one-off solution you 
suggest, but instead of calling refresh directly from the 
renderView(ViewPath) method you're calling back into CoreGUI who will 
then call refresh as appropriate - it still requires developers 
implementing RefreshableViews to remember to call something manually 
after the fact.

Again, anything that isn't enforced by contract can be too easily 
forgotten.  I'd be in favour of any change that forced implementers to 
be aware of this potential and/or for it to be automagically done for 
them at some infrastructure level.
> So, I'd be inclined to do it the former way, rather than revamping the
> infrastructure.
The fact that 90% of our views directly or indirectly extend Table does 
help to consolidate the problem.  I guess that makes the workaround 
acceptable for now.  Let's provide AMPLY documentation in the 
BookmarkableView and/or RefreshableView interfaces to describe the 
issues here, and capture the results of this thread so that future 
implementers know what they need to do (even if it isn't enforced by 
contract).

-joseph

PS - I just noticed in AbstractSectionedLeftNavigationView:

if (this.currentContent instanceof BookmarkableView) {
    ((BookmarkableView) 
this.currentContent).renderView(viewPath.next().next());
}
if (this.currentContent instanceof RefreshableView) {
    ((RefreshableView) this.currentContent).refresh();
}

I presume we'll want this block of code to use the same style as what 
you came up with for AbstractTwoLevelTabSetView?


More information about the rhq-devel mailing list