Community
Participate
Working Groups
Created attachment 83027 [details] patch The scenario is like this: When user right on a virtual node like table or column, and click refresh action, it's parent node, say schema, will be refreshed also, because model content need to be reretrieved. After this, all it's children(table, store procedure...) nodes will be collapsed. This is not a user friendly behavior if user expanded several nodes before the refresh. So we have to add a flag(boolean isUIRefreshNeeded) to refresh behavior to make it know if UI need to be refreshed, if not, only model refresh will be executed, so that no virtual nodes will be collapsed.
Hi Cong, Could you further describe how to maintain the integrity of the information displayed would we go along with your changes? The patch does not seem to account for status display (out of sync decoration), or numerous consumers of the Model Base that initialize their actions from a UI selection. As is, I do not agree with the patch. Thanks, ~Loic
(In reply to comment #1) > Hi Cong, > Could you further describe how to maintain the integrity of the information > displayed would we go along with your changes? The patch does not seem to > account for status display (out of sync decoration), or numerous consumers of > the Model Base that initialize their actions from a UI selection. > As is, I do not agree with the patch. > Thanks, > ~Loic Hi Loic, The patch doesn't change the current API, so the current implementations of model won't be affected. The new introduced interface ICatalogObjectRefreshAdapter is for controlling whether or not UI needs to be refreshed. So if model consumers could change their catalogObjects' implementation to implementing ICatalogObjectRefreshAdapter, and pass 'false' as the value of parameter 'isUIRefreshNeeded', they will also take the benefit of refreshing without collapsing. But there is no side affect if they keep their current implementation. For status display, do you mean display a text beside node to reflect their sync status? I think the feature is not there at the first place, so if we decide to add it, we can create another entry for it. And for numerous consumers, I think you mean multiple users refresh the same node at the same time? Sorry I don't understand it quite clearly, if you mean multithread issue, isn't it a database level problem while not a UI level? The patch is for dealing with the org.eclipse.datatools.connectivity.sqm.server.internal.ui.explorer.actions.popup.RevisedRefreshAction mainly, and it can be used at other places in the futher once it is applied. Thanks. Cong Chen
(In reply to comment #2) > (In reply to comment #1) > > Hi Cong, > > Could you further describe how to maintain the integrity of the information > > displayed would we go along with your changes? The patch does not seem to > > account for status display (out of sync decoration), or numerous consumers of > > the Model Base that initialize their actions from a UI selection. > > As is, I do not agree with the patch. > > Thanks, > > ~Loic > Hi Loic, > The patch doesn't change the current API, so the current implementations of > model won't be affected. > The new introduced interface ICatalogObjectRefreshAdapter is for controlling > whether or not UI needs to be refreshed. So if model consumers could change > their catalogObjects' implementation to implementing > ICatalogObjectRefreshAdapter, and pass 'false' as the value of parameter > 'isUIRefreshNeeded', they will also take the benefit of refreshing without > collapsing. But there is no side affect if they keep their current > implementation. > For status display, do you mean display a text beside node to reflect their > sync status? I think the feature is not there at the first place, so if we > decide to add it, we can create another entry for it. > And for numerous consumers, I think you mean multiple users refresh the same > node at the same time? Sorry I don't understand it quite clearly, if you mean > multithread issue, isn't it a database level problem while not a UI level? > The patch is for dealing with the > org.eclipse.datatools.connectivity.sqm.server.internal.ui.explorer.actions.popup.RevisedRefreshAction > mainly, and it can be used at other places in the futher once it is applied. > Thanks. > Cong Chen I think I missed a point, ICatalogObjectListener was refacted. Yes, consumers need to change their implementation if they implemented this interface. But I noticed that only very limited classes such as content providers implemented it, so it should not bring many refactors, and additionally, refactor should be quite simple, consumers can even ignore parameter 'isUIRefreshNeeded' if they don't want to use it. Compared to the benefit, I think it is acceptable. Additionally, I've updated ICatalogObjectRefreshAdapter, a new parameter 'modelRefreshContext' is introduced. It gives user ability to control which part of model he wants to reload. For example, user can only reload tables while not all elements of a schema or database. Please advise. Thanks. Cong.
Parameter 'modelRefreshContext' is mainly used on Schema for now, since there maybe large number of elements under a schema object, this machenism can avoid performance issues of reload. Take an example, public void refresh(boolean isUIRefreshNeeded, String modelRefreshContext) { resetLoadFlag(modelRefreshContext); RefreshManager.getInstance().refresh(this, isUIRefreshNeeded); } public void refresh() { resetLoadFlag(null); RefreshManager.getInstance().referesh(this); } private void resetLoadFlag(String modelRefreshContext){ if(modelRefreshContext == null || modelRefreshContext.equals(GroupID.Parameter){ synchronized (parametersLoaded) { if (parametersLoaded.booleanValue()) { setReturnScalar(null); parametersLoaded = Boolean.FALSE; } } } if(modelRefreshContext == null || modelRefreshContext.equals(GroupID.ResultTable){ synchronized (resultTableLoaded) { if (resultTableLoaded.booleanValue()) { setReturnTable(null); resultTableLoaded = Boolean.FALSE; } } } } Cong.
Created attachment 83219 [details] patch updated(parameter modelRefreshContext is introduced)
(In reply to comment #5) > Created an attachment (id=83219) [details] > patch updated(parameter modelRefreshContext is introduced) hi Loic, I've updated the entry, can you help to take a review? Thanks. Cong.
Cong, I don't understand the premise of this patch. The UI is always supposed to reflect the current state of the model. If this patch is applied then an adopter will be able to make model changes that are not reflected in the UI which would be a bug. I don't see any case where this behavior is desirable.
(In reply to comment #7) > Cong, > I don't understand the premise of this patch. The UI is always supposed to > reflect the current state of the model. If this patch is applied then an > adopter will be able to make model changes that are not reflected in the UI > which would be a bug. I don't see any case where this behavior is desirable. hi Larry, Please consider such case: If user right click on a virtual node such as Tables/Stored Procedure, and click 'refresh', the action will follow such process: 1. Get parent of the virtual node, say Schema A. 2. Refresh Schema A, which brings model refresh and UI refresh. 3. UI node of Schema A is collapsed. Now if user expands Schema A -> Tables, he will see updates if there is any. But there is a problem, before the refresh action, if user has expanded several virtual nodes and worked on them, the UI refresh of Schema A will collapse all of them and make user lose information. I think this is not very user friendly. The patch gives a choice to refresh model only and UI of virtual node while not Schema A, so that only the specified virtual node is refreshed and no other expanded virtual nodes are affected.
(In reply to comment #8) > (In reply to comment #7) > > Cong, > > I don't understand the premise of this patch. The UI is always supposed to > > reflect the current state of the model. If this patch is applied then an > > adopter will be able to make model changes that are not reflected in the UI > > which would be a bug. I don't see any case where this behavior is desirable. > > hi Larry, > Please consider such case: If user right click on a virtual node such as > Tables/Stored Procedure, and click 'refresh', the action will follow such > process: > 1. Get parent of the virtual node, say Schema A. > 2. Refresh Schema A, which brings model refresh and UI refresh. > 3. UI node of Schema A is collapsed. > Now if user expands Schema A -> Tables, he will see updates if there is any. > But there is a problem, before the refresh action, if user has expanded > several virtual nodes and worked on them, the UI refresh of Schema A will > collapse all of them and make user lose information. I think this is not very > user friendly. The patch gives a choice to refresh model only and UI of virtual > node while not Schema A, so that only the specified virtual node is refreshed > and no other expanded virtual nodes are affected. > Hi Cong, Unfortunately, Refresh is not a feature where we can pick and choose what should be refreshed, while the users might have only refreshed the Tables folder, some indexes might have also changed, some Stored Procedures might have had to be updated, some privileges information might need to also be refreshed, etc... I believe it would be better to not allow refresh if this could have some undesirable side effects. Let's add some extensibility that will provide some state for some objects and allow menus to execute some pre-validation. Doing this, we could envision that our extension might have to report a friendly message, letting the user know that the actions cannot be performed because of such and such editors. -> This will be used to monitor behaviors for context-menus. -> This might also be used to provide appropriate Explorer decoration.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > Hi Cong, > Unfortunately, Refresh is not a feature where we can pick and choose what > should be refreshed, while the users might have only refreshed the Tables > folder, some indexes might have also changed, some Stored Procedures might have > had to be updated, some privileges information might need to also be refreshed, > etc... > I believe it would be better to not allow refresh if this could have some > undesirable side effects. > Let's add some extensibility that will provide some state for some objects and > allow menus to execute some pre-validation. Doing this, we could envision that > our extension might have to report a friendly message, letting the user know > that the actions cannot be performed because of such and such editors. > -> This will be used to monitor behaviors for context-menus. > -> This might also be used to provide appropriate Explorer decoration. hi Loic Yes, you are right, but I think if user wants to see all changes in UI, he can refresh schema/database node while not table node. If user refreshes table only, which means he only wants to see changes of table, I think we should let user see what he wants to see. And this refresh behavior is also convienient with other popular navigators like 'windows explorer'/'package explorer'... So I believe it makes user more comfortable. Removing 'Refresh' may be not acceptable if I am a user :) I am quite interested in the 'state' you mentioned, does it mean that if user triggered some actions in context menu which can't be executed on current selected object at the time, we should show some error messages? And some decorators are displayed for reminding user to complete the object? Cong.
*** Bug 203051 has been marked as a duplicate of this bug. ***
Could we also look at a simpler approach? If I, for example, have multiple schemas expanded in the DSE and I expand one, I would expect the others to collapse and perhaps it would remember and expand the one item that I clicked refresh on. Alternatively, I believe we can get the expanded state for each item in the tree when they do a Refresh and then re-expand those items that were expanded initially. But refresh itself may act differently based on the catalog loaders used to populate the tree. For instance, my ASA loader may not work quite the same as my Derby loader, so we may not have the necessary levels of granularity to control what can and can't be refreshed/updated visually in the tree. I tend to agree with Larry in this case. Refresh refreshes everything, as it's difficult to know where changes may have occurred elsewhere in the model. But we may be able to put some smarter UI handling in to restore the state as best we can (i.e. some nodes that were expanded may have been deleted, so we ignore those) and simply re-expand them. Thoughts? --Fitz
(In reply to comment #12) > Could we also look at a simpler approach? If I, for example, have multiple > schemas expanded in the DSE and I expand one, I would expect the others to > collapse and perhaps it would remember and expand the one item that I clicked > refresh on. Alternatively, I believe we can get the expanded state for each > item in the tree when they do a Refresh and then re-expand those items that > were expanded initially. > But refresh itself may act differently based on the catalog loaders used to > populate the tree. For instance, my ASA loader may not work quite the same as > my Derby loader, so we may not have the necessary levels of granularity to > control what can and can't be refreshed/updated visually in the tree. > I tend to agree with Larry in this case. Refresh refreshes everything, as it's > difficult to know where changes may have occurred elsewhere in the model. But > we may be able to put some smarter UI handling in to restore the state as best > we can (i.e. some nodes that were expanded may have been deleted, so we ignore > those) and simply re-expand them. > Thoughts? > --Fitz Brian, I think there is little usability improvement if we only re-expand node that was clicked refresh on, and there may be a DSEQuake if we re-expand all expanded nodes while not keep them from collapsing if there are hundreds of database objects under a virtual node. For model refresh, actually we only provide an interface/chance to consumers, ModelRefreshContext can be anything, I think how to implement and use it is totally under their control. For instance, consumer can refresh all by passing NULL as ModelRefreshContext or just ignore it. I agree that Refresh refreshes everything, but that should only happens in model. If user choose to refresh a TableNode while not Schema/Database in UI, I see no reason to refresh the whole UI elements under Schema/Database. Additionally, since ICatalogObjectListener has already got consumers, is API change of ICatalogObjectListener an issue? If it is, I think we can figure out another way to do this. But as I mentioned before, the amount of consumer is very limited, as I observed, only database content providers such as SybaseASEContentProvider implement it. Did I miss something else? Please advise. Cong.
Cong, I don't believe that disconnecting the model from the visual representation in the tree really buys us anything. The idea is that the tree *IS* a representation of the model and if the model changes, the tree changes. If we start picking and choosing what gets visually displayed vs. what the data actually looks like under the covers, I think we're not maintaining the contract with the user. As such, simply remembering the node the user "refreshed" and re-expanding it may not be a good solution either. So we're at a bit of an impasse here. I don't think we can go the way you're proposing and you've pointed out the flaws with my own approach... Loic? Larry? Any suggestions here? Or do we just let it be? --Fitz
Setting target milestone to future, as we don't have a workable solution and can't predict when it will appear in the product.
