Bugzilla – Full Text Bug Listing
| Summary: | [call hierarchy] update history when a top level item is removed from view | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Deepak Azad <deepakazad> | ||||||||||
| Component: | UI | Assignee: | Markus Keller <markus.kell.r> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | daniel_megert, markus.kell.r, raksha.vasisht | ||||||||||
| Version: | 3.7 | ||||||||||||
| Target Milestone: | 3.7 M5 | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
After step 4 (remove from view), the history is not updated. While this is not entirely incorrect, you can argue that removing a top level item from the view should be reflected in the history. At the very least, if an item is explicitly added back, it should be added back, even though it might have been removed at some point in the past. (In reply to comment #1) > After step 4 (remove from view), the history is not updated. While this is not > entirely incorrect, you can argue that removing a top level item from the view > should be reflected in the history. The more I think about it, I think the right solution is to update history when a top level item is removed from view. Just like adding a top level item (Ctrl+drag) updates history. Which build ID? (In reply to comment #3) > Which build ID? I20110124-1800 > The more I think about it, I think the right solution is to update history when
> a top level item is removed from view. Just like adding a top level item
> (Ctrl+drag) updates history.
That sounds reasonable to me. Raksha, what do you think?
I guess it would also be better if the CH updated the current history element when the user adds elements to the view (rather than adding a new entry each time).
(In reply to comment #5) > I guess it would also be better if the CH updated the current history element > when the user adds elements to the view (rather than adding a new entry each > time). I would probably prefer to add a new entry each time - History is more detailed/accurate, and this could be useful at times. - Does a new entry hurt in any way ? Will there be a lot of unnecessary clutter in the history ? Is it expensive to add a new entry ? Reasons for adding new history entries: - most accurate, zero loss of most recent state Reasons against: - clutters history with less relevant input updates - increases danger of losing interesting entries This is purely a UI decision that should be based on how we expect the feature to be used. I think the history is mostly used to look at something that's not directly related to the current input elements. Adding/removing elements is a quick operation that tweaks an existing input in order to improve the shown data. Therefore, I consider the last state of an input as superior to the states before. In contrast, the value of unrelated older inputs is on the same scale as the last state of an add/remove sequence. (In reply to comment #7) > Reasons for adding new history entries: > - most accurate, zero loss of most recent state > > Reasons against: > - clutters history with less relevant input updates > - increases danger of losing interesting entries > > This is purely a UI decision that should be based on how we expect the feature > to be used. I think the history is mostly used to look at something that's not > directly related to the current input elements. Adding/removing elements is a > quick operation that tweaks an existing input in order to improve the shown > data. Therefore, I consider the last state of an input as superior to the > states before. In contrast, the value of unrelated older inputs is on the same > scale as the last state of an add/remove sequence. Updating the current history item is acceptable in case something is added, but leads to loss of data if something is removed. e.g. what happens when there are 2 top level elements in the CH view, and one of them is removed - the history entry now shows only 1 item ? I tend to agree with Markus. For me the history should get a new item each time I replace the current input entirely. When I modify the current input it should modify the current history entry. Created attachment 187620 [details] Patch (In reply to comment #7) > Reasons for adding new history entries: > - most accurate, zero loss of most recent state > > Reasons against: > - clutters history with less relevant input updates > - increases danger of losing interesting entries > > This is purely a UI decision that should be based on how we expect the feature > to be used. I think the history is mostly used to look at something that's not > directly related to the current input elements. Adding/removing elements is a > quick operation that tweaks an existing input in order to improve the shown > data. Therefore, I consider the last state of an input as superior to the > states before. In contrast, the value of unrelated older inputs is on the same > scale as the last state of an add/remove sequence. I agree that the history should have only the latest entry and not separate entries after every addition/removal since the history shows utmost 2 entries and there's no way to differentiate between the multiple entries if the first two elements are same and it clutters the dialog with duplicate entries. The latest patch fixes this as well as step 5 in the example in comment #0 and updates the history after an element is removed from view. Markus, could you pls have a look? (In reply to comment #10) > Created attachment 187620 [details] [diff] > Patch The patch fails when - Open CH on a method - Remove from view (delete) the method => Error dialog comes up - ""Delete" did not complete normally. Please see the log for more information." You have to delete the history entry itself in this case. (Which I think is loss of data and something we should not do.) Created attachment 187629 [details] Patch 2 (In reply to comment #11) Corner case. Fixed by not updating the history in case the last element is removed (like it was in 3.6). I also had to adapt CallHierarchyViewPart#addInputElements(IMember[]) to make sure it works fine when you add an element to the empty view again. RemoveFromViewAction: - in #run(), "new ArrayList(Arrays.asList(inputElements))" saves a copying op. - #getInputElements(ISelection) -> #getSelectedElements() (In reply to comment #12) > Created attachment 187629 [details] [diff] > Patch 2 > > (In reply to comment #11) > Corner case. Fixed by not updating the history in case the last element is > removed (like it was in 3.6). Well, the original problem still remains - Ctrl+drag does not work after all top level items are removed from view :) Either the history entry has to be removed, or on Ctrl+drag the dragged item(s) must be shown even though it was removed in past. (In reply to comment #12) > Created attachment 187629 [details] [diff] > Patch 2 > > (In reply to comment #11) > Corner case. Fixed by not updating the history in case the last element is > removed (like it was in 3.6). Yeah I had done the same in my fix that I almost attached... > I also had to adapt > CallHierarchyViewPart#addInputElements(IMember[]) to make sure it works fine > when you add an element to the empty view again. > > RemoveFromViewAction: > - in #run(), "new ArrayList(Arrays.asList(inputElements))" saves a copying op. > - #getInputElements(ISelection) -> #getSelectedElements() Alright, Patch 2 looks good. Committed to HEAD. Thanks Markus! > Alright, Patch 2 looks good. Committed to HEAD. Thanks Markus! No it is not, see comment 13. Reopening... (In reply to comment #15) > > Alright, Patch 2 looks good. Committed to HEAD. Thanks Markus! > No it is not, see comment 13. Hmmm dragging the same elements doesn't work anymore! Will try to clear the history altogether when the last element is removed.. Patch coming up.. Created attachment 187632 [details] Patch on top of Patch_2 (In reply to comment #17) > (In reply to comment #15) > > > Alright, Patch 2 looks good. Committed to HEAD. Thanks Markus! > > No it is not, see comment 13. > > Hmmm dragging the same elements doesn't work anymore! Will try to clear the > history altogether when the last element is removed.. Patch coming up.. Patch attached and committed to HEAD. Added changes to clear the history on removing the last element from the view.. ctrl+drag works fine afterwards on the empty view. comment #18 Contd.. We do not delete the entry but the history is cleared when a single input is removed from the view showing the empty page. The view is still open so the input can be dragged and dropped again on the same view. Marking it resolved.. feel free to test/reopen if required. and now I get the same error mentioned in comment 11 > Patch attached and committed to HEAD. Added changes to clear the history on > removing the last element from the view.. ctrl+drag works fine afterwards on > the empty view. Plus do you clear the entire history? (In reply to comment #20) > and now I get the same error mentioned in comment 11 > I cannot reproduce that :/ I see the element being removed and an empty page shown.. I'll take care of this. The right fix is my patch 2 + some tweaks to make sure a removed input element shows up again when the user Ctrl+drags it to the view. Created attachment 187636 [details] Patch 4 This patch is against HEAD. It reverts comment 18 and implements comment 22. Released to HEAD. Deepak, please check HEAD. Its much better now, however a small issue remains - Open CH on foo() - Remove foo() => history contains 1 item 'foo()' - Ctrl+Drag bar() to CH view => view contains 'bar()' and history contains 1 item 'foo(), bar()'. This is not consistent with 'history should have only the latest entry' policy. This is a corner case, and should not happen too often in practice. Markus, your call if you want to fix this or not. (In reply to comment #25) Thanks, fixed in HEAD of CallHierarchyViewPart#addInputElements(IMember[]). Works nicely now. Verified in I20110127-2034. One scenario is a little bit unexpected, see bug 335657. . |
--------------------------------------- package p; class A { void foo(Object a) { bar(); } void bar() { foo(this); } } --------------------------------------- 1. Empty CH view 2. Select foo in Outline view and drag to CH view => CH view looks like -foo + bar 3. Select bar in Outline view and Ctrl+drag to CH view => -bar + foo -foo + bar 4. Remove from view (delete) bar => -foo + bar 5. Select bar in Outline view and Ctrl+drag to CH view => Not good ! -foo + bar 6. Refresh View (view toolbar button) => -bar + foo -foo + bar