Community
Participate
Working Groups
N20090419-2000 Follow-up to bug 183602 (regression compared to 3.4). IWorkbenchPage#openEditors(IEditorInput[], ..) should add the non-materialized editors at the front of the editors MRU list. Currently, when I select and open multiple elements in the Open Type dialog, the first editor is created and comes to front (good), but the others are added at the end of the MRU list. This causes several problems: - 'Close' on the topmost editor does not reveal the next of the opened types. Instead, I am thrown back to the previously active editor. - If there's not enough space to show all editor tabs, then the other freshly opened editors are not visible (they are hidden in the tab extension menu). - Ctrl+F6 does not easily let me switch to the other editors.
Oleg, could you please look into this?
Created attachment 133178 [details] Patch Good catch. The attached patch fixes inconsistencies you found - except for the: "- If there's not enough space to show all editor tabs, then the other freshly opened editors are not visible (they are hidden in the tab extension menu)." I am not sure how to implement that one in a reasonable way.
Created attachment 133179 [details] Updated patch The first patch included some extra changes, please use this one.
Boris, can you look at this for M7? This is new in 3.5 and should be fixed for the release.
Released to HEAD. Thanks for the reminder, Dani! It's a crazy day...
Almost fixed. >"- If there's not enough space to show all editor tabs, then the other freshly >opened editors are not visible (they are hidden in the tab extension menu)." > >I am not sure how to implement that one in a reasonable way. Markus point was that the newly opened editor tabs should be shown in favor of existing ones. Take a look at the example screen shots.
Created attachment 133538 [details] Initial state
Created attachment 133539 [details] Expected / 3.4 sate after opening 3 editors
Created attachment 133540 [details] Unexpected state after opening 3 editors
(In reply to comment #6) > Markus point was that the newly opened editor tabs should be shown in favor > of existing ones. See the comment 2.
>See the comment 2. I read that but since it worked in 3.4 I would assume there is already an algorithm that does this and which can be used here too.
(In reply to comment #11) > I read that but since it worked in 3.4 I would assume there is already an > algorithm that does this and which can be used here too. Opeining multiple editors is a new functionality added in 3.5. I don't know why Markus used word "regression". Maybe he was comparing it to opening multiple editors from IMemento's on startup? That works from a "clean slate" and does not have to deal with tab space being occupied already. (I added bug 274035 to see if we can add more control over the tab visility in 3.6.) Markus, is that correct? Or is there some "opening multiple editors without materialization" path that worked in 3.4 and does not work in 3.5?
Well, it is a regression for the user who does not really care about internals.
(In reply to comment #13) > Well, it is a regression for the user who does not really care about internals. Everybody who used 3.4 APIs behaves the same as in 3.4. If you use the new API to open multiple editors in a "quick" way then you get the new behaviour with tabs. But you do have to use the new API for that. Did you change JDT code to use "open multiple editors without materalization"?
> Did you change JDT code to >use "open multiple editors without materalization"? Yes, not only Open Type but also Open Resource uses the new API and we started to use the new API, expecting to get the same behavior for the user but with the performance gain of not creating all the editors.
(In reply to comment #15) > > Did you change JDT code to > >use "open multiple editors without materalization"? > Yes, not only Open Type but also Open Resource uses the new API and we started > to use the new API, expecting to get the same behavior for the user but with > the performance gain of not creating all the editors. > We did not foresee this but unfortunately cannot implement the same behavior. I suggest that you revert your use of the new API (which should work without problems for the Mylyn use case). We can fix bug 274035 in 3.6 after which you can start using the new API without introducing regressions. The reason for the regression is that CTabFolder manages the MRU list, and there is no way for us to tweak it using API to get the new tabs to show up.
There's another issue with the fix: while the MRU list is now better, it inverts the order of files i.e. before this bug was fixed it was f1, f2 and now they appear as f2, f1: 3.4.x/correct: f1 [f2] before this fix: [f1] f2 with this fix: f2 [f1]
Created attachment 133780 [details] Fix This fixes all the issues initially raised.
>This fixes all the issues initially raised. Plus the issue mentioned in comment 17.
Sorry, my patch is useless as it activates all editors.
Let me know this week whether you plan to work on this for 3.5. If not, I'll back out the changes next week.
I believe we should try to fix the problem described in comment 17. Oleg, could you look into this? As for the editors not showing up as tabs because of the built-in MRU logic in CTabFolder, I don't think this is something we can fix for 3.5.
Created attachment 134969 [details] Better Fix This fix makes the order right again and also fixes the tab item issue. If we think it is too dangerous to change PresentablePartFolder.insert(IPresentablePart, int) at this point, we might be able to select the tab item at a higher level place.
Created attachment 134970 [details] Better Fix (same as before but fixed broken comment)
(In reply to comment #17) > There's another issue with the fix: while the MRU list is now better, it > inverts the order of files i.e. before this bug was fixed it was f1, f2 and now > they appear as f2, f1: > 3.4.x/correct: f1 [f2] > before this fix: [f1] f2 > with this fix: f2 [f1] I don't think this is correct. Using 3.4 as a reference is strange for the functionality added in 3.5. What happens now: say, we have multi-selected 3 files: text1 text2 text3 and used #openEditors() method to open all 3 of them at once. The File MRU list will be: text1, text2, text3. The active tab will be "text1". The tab order will be: text1 (active) -> closed: -> text2 -> closed: -> text3. What you are asking about is the opposite: text3 to be active, and MRU lists be text3, text2, text1. To me, the current behavior is more intuitive. Plus, the Javadoc on IWorkbenchPage#openEditors() sais: "Only the editor constructed for the first input gets activated." With the proposed patch it is the last editor passed in that gets activated. (Which, by the way, breaks JUnits in IWorkbenchPageTest.) ============ For the tab visibility part of the patch (PresentablePartFolder), that looks interestings. I don't know that code so can't comment on side effects, but it seems to work well for the rudimentary testing I've done.
>I don't think this is correct. Using 3.4 as a reference is strange for the >functionality added in 3.5. Oh well. The one and only purpose was to get a performance effective way to replace the existing code that materialized the editors and not to discuss whether the many year old behavior is good or not. I can't see any bug report/comment that asked to change that behavior and hence cannot understand why the new API decided to change this. Boris, what's the reason to change this in the new API? Please decide asap how you intend to proceed as otherwise we have to go back to the old API that materializes the editors.
>For the tab visibility part of the patch (PresentablePartFolder), that looks >interestings. I don't know that code so can't comment on side effects, but it >seems to work well for the rudimentary testing I've done. It's the right direction but we need to call setSelection at some other place: I ran with the patch for quite some time now and see side-effects, e.g. when restoring view stacks. This is probably also what the failing test detected.
(In reply to comment #26) > >I don't think this is correct. Using 3.4 as a reference is strange for the > >functionality added in 3.5. > Oh well. The one and only purpose was to get a performance effective way to > replace the existing code that materialized the editors and not to discuss > whether the many year old behavior is good or not. I can't see any bug > report/comment that asked to change that behavior and hence cannot understand > why the new API decided to change this. > > Boris, what's the reason to change this in the new API? No particular reason, other than that the new API Javadoc makes sense as is. I don't think it is more intuitive to say that "only the editor for the last element in the array is materialized". I think it's up to you to decide in which order you want the editors to appear, but this is something you can do easily on your end (just reverse the array if needed).
(In reply to comment #27) > It's the right direction but we need to call setSelection at some other place: > I ran with the patch for quite some time now and see side-effects, e.g. when > restoring view stacks. This is probably also what the failing test detected. The change in PresentablePartFolder is interesting but seems to risky at this point. We can release something like this early in 3.6 and if it works well backport it to 3.5.1. What do you think?
>I think it's up to you to decide in which order you want the editors to appear, >but this is something you can do easily on your end (just reverse the array if >needed). OK. >The change in PresentablePartFolder is interesting but seems to risky at this >point. We can release something like this early in 3.6 and if it works well >backport it to 3.5.1. What do you think? As stated in my previous comment we'd first have to find a better place. My current "fix" breaks things. So yes, we probably have to move this to 3.5.x or 3.6. For 3.5 I'll remove the usage of the new API in the Open * dialogs because it's very annoying that closing newly opened editors doesn't activate them in the right order.
Moving to 3.6. Sorry that the new API is not useful for you (I hope it is useful for Mylyn at least though). We can try to get this into 3.5.1, by addressing the MRU issue early in 3.6. Btw, it would be useful to document what kind of side-effects you saw while running with the patch, and how to reproduce them. I am hoping that the change to PresentablePartFolder is close to the solution.
>Btw, it would be useful to document what kind of side-effects you saw while >running with the patch, and how to reproduce them. I am hoping that the change >to PresentablePartFolder is close to the solution. Sure. When starting a fresh workspace it has the 'Hierarchy' view tab in front with the not yet created view instead of the 'Package Explorer'. Boris, I suggest that for consistency we also revert the change to 'Open Resource'. I can make a patch for you to review if you agree.
(In reply to comment #32) > Boris, I suggest that for consistency we also revert the change to 'Open > Resource'. I can make a patch for you to review if you agree. Good point. Thanks for the offer!
Created attachment 143938 [details] Patch v2 I think if we use CTabFolder#showItem() instead of CTabFolder#setSelection() we'll reduce extra processing. The attached Patch v2 produces the desired behavior for editor tabs and so far I haven't seen side effects. The bulk of changes in the patch are propagating CTabFolder#showItem() to the level at which we can use it. (As Boris mentioned, the ordering of editors - "first in - first out" vs. "first in - last out" can be changed by filling argument arrays in reverse order. To me, the API makes sense as it is, so this patch is concentrated on the producing correct tabs for editors.) Boris, could you have a look at the patch?
Kevin, as the master of the CTabFolder, could you review the Patch v2?
The patch does not completely fix the problem: while the editors are now in the correct order, the editor list that appears when clicking the chevron still has wrong/scrambled order. >(As Boris mentioned, the ordering of editors - "first in - first out" vs. >"first in - last out" can be changed by filling argument arrays in reverse >order. My problem is that currently neither LTR nor RTL is specified anywhere in the API. The only thing that I see currently specified is that the first one from the given array is activated. Or did I miss something?
(In reply to comment #36) > The patch does not completely fix the problem: while the editors are now in the > correct order, the editor list that appears when clicking the chevron still has > wrong/scrambled order. Hmm... that's not what I see. With Patch v2, when I use the same order (i.e., rememeber to reverse array for multi-open) the drop-down lists from the chevron look the same. (To me, the ordering in the drop-down makes no sense, but that is a dfferent subject that has been widely debated before. See bug 58157, bug 68684 "MRU ordering is considered a feature, not a bug" :-), bug 168379, and I am sure many others.)
Created attachment 147360 [details] Editors opened on-by-one; order is in red letters to the right
Created attachment 147361 [details] Editors opened using the new API
ping Dani, could you check the editor drop-down order (comment37) ? From what I gothered, the chevron's drop-down has two lists: visible tabs (in normal font) and invisible tabs (in the bold font). Both lists are sorted alphabetically and do not follow MRU order. Boris, could you review "Patch v2"?
>(i.e., rememeber to reverse array for multi-open) Right, very unnatural and unexpected and hence I forgot to do that ;-). I retrieved OpenResourceHandler rev. 1.6 from the repository which does not yet "fix" the order. With that changed, the patch works.
Ping Ping Boris, could you review "Patch v2"?
Patch looks good.
Created attachment 153115 [details] Patch v2 updated Updating patch as the old one is out of synch.
Thank you! "Patch v2 updated" is applied to CVS head. This should fix the MRU problem when files are passed in the order described in the new APIs.
I can still not use that API as it does not transfer the keyboard focus to the editor which is in front :-( I filed bug 296477 to track this.
Verified in I20091207-1800.
I reopen the bug, because the problems described in comment #0 still occur in Eclipse 4.4 when trying to fix bug 178229 (by applying a patch attached to that bug).
Mass move to 4.7 as M7 is approaching. Please move back in case you are planning to fix it for Neon.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
Still relevant.
