Community
Participate
Working Groups
Build Identifier: 3.7.2 If we restart the workspace, a cloned view will not remember the setting that was chosen in the view menu. If fact, restarting the workspace seems to make all view take the NumberFormat that was deactivated last - deactivating a view triggers the saving of memento. Details is in https://bugs.eclipse.org/bugs/show_bug.cgi?id=370462 Comment 110. Reproducible: Always
Created attachment 211501 [details] patch
Hi Winnie, On the surface this makes sense however in practice I think it could actually make things worse: The second view IDs are assigned using a counter and they are re-used. What this means is that if you: * clone a view, * change its settings * close the view * close WB * restart WB * open first view * modify its settings * clone a new view * this new view will inherit the settings from the previous cloned view, not from the first view (I haven't actually tried this, but logically it should happen) I.e. the settings in new cloned views could be somewhat unpredictable. It may not be desirable for all views to take the last saved setting, but at least it's fairly predictable. Do you agree/disagree? Have any suggestions?
(In reply to comment #2) > I.e. the settings in new cloned views could be somewhat unpredictable. It may > not be desirable for all views to take the last saved setting, but at least > it's fairly predictable. Do you agree/disagree? Have any suggestions? Good point. But the current situation seems to be more unpredictable than would appear. And this is without even restarting the workspace. The memento is being saved not only on disposal of a view but also on deactivation. So, weird things can happen. For example: - launch a DSF debug session - look at variables view. Say it has the 'default' format - clone the view: the new one will have the 'default' format - close the cloned view - change the variables view format in the view menu to 'binary' - clone the view: the new one will still have the 'default' format (memento not saved yet) - close the cloned view - hide the variables view with another view (memento gets saved) - make variables view visible - clone the view: the new one will have the 'binary' format In bug 370462, we will try not to use the number format of the memento when a view is cloned/opened. This should give a predictable behaviour. I'm not sure if a more generic solution would be needed in platform, as this problem could happen to others.
I think the suggested change makes sense if we could recognize when a view is being closed by the user (as opposed to IDE shutting down). In this case, we could erase the memento when closed manually. I thought of another approach also: instead of using a counter to generate a secondary ID, use an UUID so that preferences are not reused by a new view. The only problem then is that without some control, we would be creating a memory leak in the preference store, so we would somehow need to keep track of old used preference IDs and delete them.
(In reply to comment #4) > I think the suggested change makes sense if we could recognize when a view is > being closed by the user (as opposed to IDE shutting down). In this case, we > could erase the memento when closed manually. When we are dealing with a single instance of a view, and the user manually closes that view, we still want the memento so as to restore the attributes of the view (columns and such). When dealing with cloned views, it gets fuzzy. Do we want to restore the attributes of that cloned view as it was last seen, or do we want to re-clone the existing view with its current attributes? And what do we want to do when restarting the workspace? Probably restore all views as they were before. This is not obvious to me. > I thought of another approach also: instead of using a counter to generate a > secondary ID, use an UUID so that preferences are not reused by a new view. That would mean that preferences would be recovered for a workspace restart, but not a re-cloning. That seems better. But then I wonder what a user will want. Say I have two variables view that I configured exactly like I want. I then close the second one by mistake. I'd like to be able to re-open it with the same config as before. On the other hand, maybe I closed the view because I want to reset its settings, and when I re-clone it, I want the default values... Maybe the best way is to mimic the behavior of the single view use-case. A view settings are persisted, period. This makes me rethink comment 2. If we apply it to a single view, the view attributes will be restored from the memento, no? I mean: * open variables view * change its settings * close the view * close WB * restart WB * open the variables view * the setting will be the ones that were set before closing the view, which could be a long long time ago. Since that is good enough for the single-view use-case, we could use the same for cloned views. Then for some settings, we could ignore the memento, and override the attribute, as we are planning on doing in CDT for the number format. But it leaves the workspace restart case un-resolved... I need to think about it more. > The only problem then is that without some control, we would be creating a > memory leak in the preference store, so we would somehow need to keep track of > old used preference IDs and delete them. Unless we feel this makes the user experience clearly better, I suggest we avoid complicated implementations :)
Let's try to make the discussion as basic as we can. We need to deal with: 1- Restarting a workspace 2- Manually creating (cloning) a view 1- Restarting a workspace I think we are starting to agree that a workspace restart should restore all its views to their individual state when the workspace exited. This particularly makes sense for multicore debug, where users may have a complex setup of multiple cloned views. They would want to be able to preserve that setup, I believe. Do we agree? Before Cloning of views was supported, this is what the memento achieved. With Cloning however, the single memento does not allow this. 2- Manually creating (cloning) a view Before Cloning was supported, correct me if I'm wrong, but a view could only be created if it had been closed (manually or workspace exit). Therefore the memento was always up-to-date, since it is saved on deactivation, and creating a view would properly restore the latest attributes from the memento. When Cloning came along, it was suddenly possible to create a view without closing/deactivating the existing one. Suddenly, the memento started being used although it might not have been saved with the latest changes. This currently gives seemingly unpredictable behavior when cloning views (see comment 3). It would good to fix that. So, what do we really want to happen when cloning? a) use the exact attributes of the view that is being cloned b) use the exact attributes of the last view that was cloned c) recover attributes from the last time this exact instance of the view was open d) don't recover attributes at all and start the view 'clean' e) something else that I didn't think of? b) I don't like, as it is not something easy to anticipate, e.g.., I clone 'variables 2', but the new view takes the attributes of 'variables 4'. Weird. c) may be the simplest, if we implement individual mementos for #1. However, Pawel is right that it might appear random to a user, if a cloned view starts with attributes that were saved long ago (see comment 2). d) is also very simple, but does not help the user much. Configuration of a view would have to be done again and again for every clone. For multicore, that may be annoying. a) is easy to understand, and helps the user re-use a configuration she took time to create for a previous view. In fact, with solution a), the user can mimic d) by simply cloning the views before making any changes to them. So, I vote for a). Of course, that is without looking at the complexity of the implementation :O What are your thoughts?
Created attachment 211610 [details] Deleting mementos, for discussion This small patch (~10 lines) builds on top of Winnie's patch, which must be applied first. From comment 6, the patch implements #1 and #2 d). Which is: - All cloned views' attributes are restored after a workbench restart (Winnie's patch) - Cloning a view will use the default attributes from the platform. This is achieved by deleting mementos when a view is closed manually, as per Pawel's suggestion. also: - Opening the main view (no secondaryId) will restore from the memento. This is to keep the behavior as it was before cloning. I didn't think we should affect people that never use cloning. This is is achieved by not deleting the memento if it is for the main view. I wasn't able to implement #2 a), which was to keep the attributes from the view being cloned. I don't know of a way to figure out which view was used when the clone button was pressed by the user. What do you think? I still haven't thought about how this affects what we want to do in CDT, but I think it gives the platform a better behavior.
(In reply to comment #6) > a) is easy to understand, and helps the user re-use a configuration she took > time to create for a previous view. In fact, with solution a), the user can > mimic d) by simply cloning the views before making any changes to them. > > So, I vote for a). Of course, that is without looking at the complexity of the > implementation :O I like a) as well, and I think there's a simple way to implement it, though it has one catch. The way to implement it would be for the clone action to copy the preference value from the view being cloned to the new clone. The catch is that the clone action, which is currently in CDT would need to access the org.eclipse.debug.ui preference store, and it would need to know the preference memento key. I guess the cleanest API impact would be to contribute the clone action to platform, even if other preojects are responsible for contributing it to the view toolbar. Do you guys agree? If so anyone have time to make the patch?
(In reply to comment #8) > (In reply to comment #6) > > a) is easy to understand, and helps the user re-use a configuration she took > > time to create for a previous view. In fact, with solution a), the user can > > mimic d) by simply cloning the views before making any changes to them. > > > > So, I vote for a). Of course, that is without looking at the complexity of the > > implementation :O > I like a) as well, and I think there's a simple way to implement it, though it > has one catch. The way to implement it would be for the clone action to copy > the preference value from the view being cloned to the new clone. The catch is > that the clone action, which is currently in CDT would need to access the > org.eclipse.debug.ui preference store, and it would need to know the preference > memento key. > I guess the cleanest API impact would be to contribute the clone action to > platform, even if other preojects are responsible for contributing it to the > view toolbar. > Do you guys agree? If so anyone have time to make the patch? I have a complete thing for all requirements, will post a patch soon...
Created attachment 211614 [details] patch To show the idea how I want to fix, subject to clean-up. I add in-line comment to explain why I change code in that way.
(In reply to comment #10) > Created attachment 211614 [details] > patch > To show the idea how I want to fix, subject to clean-up. I add in-line comment > to explain why I change code in that way. This patch does not need "Created attachment 211610 [details] Deleting mementos, for discussion". And I obsolete that patch I put on Feb 23 that make PREF_STATE_MEMENTO include secondaryId.
I'm sorry, I couldn't get the patches to apply, and I'm rather confused by their logic. For example, I don't see where saveStateInSharedMemento() is called from. Before investing more time into it though, do you see any problem with contributing the clone view logic to platform as I suggested in comment #8. It seems that it would afford a much simpler implementation.
(In reply to comment #8) > I like a) as well, and I think there's a simple way to implement it, though it > has one catch. The way to implement it would be for the clone action to copy > the preference value from the view being cloned to the new clone. From what I can see, the clone button is associated with OpenNewViewActionDelegate. There seems to be only one instance of this class per type of debug view, e.g, only one for all the different clones of the variables view. How would that class figure out from which clone it was called? It seems to know which view it is associated with from the call to OpenNewViewActionDelegate.init(IViewPart), but this method is only called once, and with the main variables view part. Maybe there is way to figure out the clone from the IAction parameter that is passed to OpenNewViewActionDelegate.run? If there is a way, then this solution sounds good. > The catch is > that the clone action, which is currently in CDT would need to access the > org.eclipse.debug.ui preference store, and it would need to know the preference > memento key. > > I guess the cleanest API impact would be to contribute the clone action to > platform, even if other preojects are responsible for contributing it to the > view toolbar. I think that is fine.
(In reply to comment #13) > How would that class figure out from which clone it was called? It seems to > know which view it is associated with from the call to > OpenNewViewActionDelegate.init(IViewPart), but this method is only called once, > and with the main variables view part. Good point. I guess a command handler would be more powerful. Unfortunately I don't have time to investigate this now.
(In reply to comment #14) > (In reply to comment #13) > > How would that class figure out from which clone it was called? It seems to > > know which view it is associated with from the call to > > OpenNewViewActionDelegate.init(IViewPart), but this method is only called once, > > and with the main variables view part. > > Good point. I guess a command handler would be more powerful. Unfortunately I > don't have time to investigate this now. Winnie, this current limitation (not having access to the cloned view where the clone button was pressed) also affects your proposed solution for CDT (bug 370462 comment 122). Do you want to try using a command handler to see if it gives us what we want? The alternative is to fall back to the solution of comment 7. I think such a solution is a good improvement on the current situation. Also, it makes the use of preferences in CDT (bug 370462), quite easy to do.
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > How would that class figure out from which clone it was called? It seems to > > > know which view it is associated with from the call to > > > OpenNewViewActionDelegate.init(IViewPart), but this method is only called once, > > > and with the main variables view part. > > > > Good point. I guess a command handler would be more powerful. Unfortunately I > > don't have time to investigate this now. > Winnie, this current limitation (not having access to the cloned view where the > clone button was pressed) also affects your proposed solution for CDT (bug > 370462 comment 122). > Do you want to try using a command handler to see if it gives us what we want? > The alternative is to fall back to the solution of comment 7. I think such a > solution is a good improvement on the current situation. Also, it makes the > use of preferences in CDT (bug 370462), quite easy to do. Guys, I know what happened. In eclipse 3.x, for the extension point org.eclipse.ui.viewActions, each view instance has its own action delegate instance. This is currently broken in eclipse 4.x M5. In other words, in 4.x, toolbar action delegate and view pull down action delegate contributed through this extension point will share the same action delegate instance between multiple view instances of the same view type. This violates the IViewActionDelegate contract in 3.x mechanism. I have code that conributes through the extension point org.eclipse.ui.menus. Since the IHandler.exectue() is given the right event object, the handler can figure out the right view it is supposed to cloning from. This approach works in both 3.x and 4.x. Since there are quite a few places in cdt using this org.eclipse.ui.viewActions to contribute actions and so affect multiple view instances of variables/registers/expressions/memory browser in 4.x, I am going to test on the lastet 4.x integration build. If the problem still exists, I will file a bug to the right place and see how e4-dev team says about it. If they plan to fix it in Juno, that will be great. If they cannot fix it, we will have to change some affected cdt code. Let's wait and see. If you want me to upload a patch using org.eclipse.ui.menus for the purpose of seeing the behavior effect of 'original proposal patch', let me know. I agree that my original proposal patch needs to be clean up and some of the things will be based on API as Pawel points out. I want to prove and show the behavior from user angle, then work out the details of APIs.
(In reply to comment #16) > Since there are quite a few places in cdt using this org.eclipse.ui.viewActions > to contribute actions and so affect multiple view instances of > variables/registers/expressions/memory browser in 4.x, I am going to test on > the lastet 4.x integration build. If the problem still exists, I will file a > bug to the right place and see how e4-dev team says about it. > If they plan to fix it in Juno, that will be great. If they cannot fix it, we > will have to change some affected cdt code. Let's wait and see. Alright, I filed Bug 372684. While we are waiting for their response, is there anything else you guys want me to try... - change action delegate to handler using org.eclipse.ui.menus? - think deep about the API that Pawel mentions?
(In reply to comment #17) > Alright, I filed Bug 372684. While we are waiting for their response, is there > anything else you guys want me to try... > - change action delegate to handler using org.eclipse.ui.menus? > - think deep about the API that Pawel mentions? Light a virtual candle offering at the bugzilla altar ;-)
(In reply to comment #18) > (In reply to comment #17) > > Alright, I filed Bug 372684. While we are waiting for their response, is there > > anything else you guys want me to try... > > - change action delegate to handler using org.eclipse.ui.menus? > > - think deep about the API that Pawel mentions? > Light a virtual candle offering at the bugzilla altar ;-) They target their fix in M7. Since my patch depends on that and it looks we have to agree on/revise some details, I think it is a bit too late for my to put all the pieces together in M7 and work the patch for this restart bug - which will then be in M8. Should I just change this 'view clone action delegate' to handler and continue the work?
(In reply to comment #19) > Should I just change this 'view clone action delegate' to handler and continue > the work? There's so many options open that I'm not sure what to recommend. M6 is API freeze so if we want to add the action and other pin+clone stuff to platform it should be now. But it sounds like you also have options to achieve desired behaviour without platform changes. If you can do that in Indigo then re-evaluate the implementation in the future, that's fine too.
(In reply to comment #20) > (In reply to comment #19) > > Should I just change this 'view clone action delegate' to handler and continue > > the work? > > There's so many options open that I'm not sure what to recommend. M6 is API > freeze so if we want to add the action and other pin+clone stuff to platform it > should be now. But it sounds like you also have options to achieve desired > behaviour without platform changes. If you can do that in Indigo then > re-evaluate the implementation in the future, that's fine too. M6 is in two weeks. If we want a change in platform we are running out of time. Here are the options as I see them: 1- fix only in CDT for now. This means that restarting the workspace will not restore cloned views to their previous settings. 2- Use the Deleting mementos solution. The workspace restart case will work, and we can implement what we need for CDT on top. However, when cloning a view, the default settings will be used. 3- Try to push forward with Winnie's final solution (but I think it needs to be explained in more details, as I didn't get it completely). To do that, we make sure things work on 3.8, while expecting bug 372684 to automatically make it work for 4.2. #3 is the more complete solution but has the risk of not working for 4.2. Winnie, what will be the impact if bug 372684 is not fixed and we went with #3? Also, this solution requires some cleanup from Winnie and some time for Pawel to review the solution. #2 is less complete, but is better than what we have now. The solution is ready and simple to understand. #1 is even more incomplete but does not change the platform. I think it must be Pawel making the call if #3 is feasible or not. If #3 is too risky, or requires too much time, I suggest we go for #2. Thoughts?
(In reply to comment #21) > 3- Try to push forward with Winnie's final solution (but I think it needs to be > explained in more details, as I didn't get it completely). To do that, we make > sure things work on 3.8, while expecting bug 372684 to automatically make it > work for 4.2. I vote for #3. I will do all these - change that particualr action delegate to handler using org.eclipse.ui.menus, since the action delegate is 'deprecated' as Paul Webster confirmed - eventually cdt code has to been changed to not using that deprecated mechanism. - define a clear api that basically says 'saveStateInSharedMemento' as illustarted in attachement #211614 patch - clean up hard-coded strings. Will post a patch tomorrow.
(In reply to comment #21) > M6 is in two weeks. If we want a change in platform we are running out of > time. Here are the options as I see them: The timing is even more tight than that. Warm up builds for Platform M6 start this Sunday. I also have a stack of other bugs to look at before then. So I think the best bet right now would be something very simple and not risky. To change API in M7 we'd need API approval, plus CDT couldn't pick it up until milestone integration time.
(In reply to comment #22) > I vote for #3. I will do all these I have concerns with this solution. Like Mark, I don't completely understand it and I think it would introduce a non-standard API extension with saveStateInSharedMemento(). I.e. VariableView is an internal class so you'd need to add an extension to IDebugView. If we do add an extension to IDebugView, I think it should be more flexible in how it allows clients to manage view state of cloned views. I.e. restore settings from shared memento vs. from view being cloned, etc. Finally I don't think I'll have time to review such an API contribution before end of week.
Created attachment 212243 [details] Solution #2 complete patch (In reply to comment #24) > > I vote for #3. I will do all these > I have concerns with this solution. I think #3 is out then. How about #2 for Juno, and we can revisit #3 for Kepler? For clarity, I attached what is needed for #2. It is 27 lines. It introduces a different memento for each cloned view (Winnie's patch); could this be a problem if we want to change this solution for Kepler?
(In reply to comment #23) > (In reply to comment #21) > > M6 is in two weeks. If we want a change in platform we are running out of > > time. Here are the options as I see them: > The timing is even more tight than that. Warm up builds for Platform M6 start > this Sunday. I also have a stack of other bugs to look at before then. So I > think the best bet right now would be something very simple and not risky. To > change API in M7 we'd need API approval, plus CDT couldn't pick it up until > milestone integration time. How about this: - If M7 does not fix 372684, change that particualr action delegate to handler using org.eclipse.ui.menus. This is cdt code, and is not a risky part. I do not know how many M's there after M7. You guys have to clearly tell whether I should change now for M6 - No new api. But must have public method 'saveStateInSharedMemento' in VariablesView so that the delegate or handler can call. This is platform code. - clean up hard-coded strings. Must do now.
(In reply to comment #25) > Created attachment 212243 [details] > Solution #2 complete patch > (In reply to comment #24) > > > I vote for #3. I will do all these > > I have concerns with this solution. > I think #3 is out then. > How about #2 for Juno, and we can revisit #3 for Kepler? > For clarity, I attached what is needed for #2. It is 27 lines. It introduces > a different memento for each cloned view (Winnie's patch); could this be a > problem if we want to change this solution for Kepler? # 2 does not work for me. Try testing all a bunch of view instances. Your approach of '// Only delete if we are not the main view of this type' did not work for general cases. I would not bet on that direction.
(In reply to comment #26) > - No new api. But must have public method 'saveStateInSharedMemento' in > VariablesView so that the delegate or handler can call. This is platform code. > - clean up hard-coded strings. Must do now. I wouldn't mind adding this as a provisional API if I understood and agreed with the approach. But I would rather not open the internal working of storing debug views' state as an API. It would reduce our flexibility to fix and change this logic in the future. Have you seriously considered my suggestion from comment #8. It would add the generic feature of cloning the variables views with a small and well defined API. It would also allow other projects/products besides CDT to leverage this feature. On the downside, it would reduce your flexibility in CDT to make changes to it. Also, I know there's quite a bit of code in CDT to support this, and it may end up being a large patch, in which case it would be out of scope for 3.8 anyway.
(In reply to comment #28) > (In reply to comment #26) > > - No new api. But must have public method 'saveStateInSharedMemento' in > > VariablesView so that the delegate or handler can call. This is platform code. > > - clean up hard-coded strings. Must do now. > I wouldn't mind adding this as a provisional API if I understood and agreed > with the approach. But I would rather not open the internal working of storing > debug views' state as an API. It would reduce our flexibility to fix and > change this logic in the future. > Have you seriously considered my suggestion from comment #8. It would add the > generic feature of cloning the variables views with a small and well defined > API. It would also allow other projects/products besides CDT to leverage this > feature. On the downside, it would reduce your flexibility in CDT to make > changes to it. Also, I know there's quite a bit of code in CDT to support > this, and it may end up being a large patch, in which case it would be out of > scope for 3.8 anyway. I wonder why pin-and-clone was not implemented like what you suggseted in comment #8 in the beggining. Did someone in the past try doing so, but it ended up that he was forced to put code in cdt?
(In reply to comment #29) > I wonder why pin-and-clone was not implemented like what you suggseted in > comment #8 in the beggining. Did someone in the past try doing so, but it ended > up that he was forced to put code in cdt? Pin implementation in CDT is CDT-specific and without pin clone is not very useful :-) So in platform we just added the cloneable flag to the view to let CDT experiment with the feature. CDT pin has matured enough, but it couldn't be contributed to platform as is because for example it relies a lot on color to identify views which is against accessibility requirements. However, with some effort, the whole pin mechanism could be contributed to platform.
Created attachment 212415 [details] try to solve
(In reply to comment #31) > Created attachment 212415 [details] > try to solve Could you explain what the patch attempts to do? I personally think it would make the review much easier.
This request should be addressed by the proposed API additions the pin and clone feature: bug 145635.
I'm marking this as duplicate of the pin and clone bug, since we'll need to add a new action+API for views to clone their settings, which will have to tie in with the comprehensive pin+clone feature. *** This bug has been marked as a duplicate of bug 145635 ***
