review of commit ea30719

Joseph Marques jmarques at redhat.com
Mon Dec 13 18:41:15 UTC 2010


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.


More information about the rhq-devel mailing list