review of commit ea30719

Ian Springer ian.springer at redhat.com
Mon Dec 13 21:25:59 UTC 2010


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

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.

So, I'd be inclined to do it the former way, rather than revamping the 
infrastructure.



More information about the rhq-devel mailing list