Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NETBEANS-5209] Document switcher popup not grouping by project on first use. #3299

Merged
merged 2 commits into from Jan 17, 2022

Conversation

mbien
Copy link
Member

@mbien mbien commented Nov 3, 2021

issue:

  1. tools -> options -> appearance -> doc tabs -> check sort opened docs by project
  2. open some files, open the tab switcher popup on the upper right corner of the editor
  3. you will notice first open does not group by projects as requested
  4. open it again... should work now

fix for document switcher popup not grouping by project on first use:

  • file owner query is now a blocking opperation with timeout
  • under normal conditions the query returns a result right away
  • if it doesn't for whatever reason it won't block the EDT due to the timeout
    • fallback is the regular tab list
  • it fixes the issue as side effect + avoids repaint flickering of the original impl

@KacerCZ
Copy link
Contributor

KacerCZ commented Nov 3, 2021

@mbien I filed this issue some time ago as https://issues.apache.org/jira/browse/NETBEANS-5209

@neilcsmith-net neilcsmith-net changed the title Document switcher popup not grouping by project on first use. [NETBEANS-5209] Document switcher popup not grouping by project on first use. Nov 3, 2021
@KacerCZ
Copy link
Contributor

KacerCZ commented Nov 3, 2021

@mbien I tested this fix with PHP project - it fixes first use after loading IDE.
Newly opened files are still sorted under "no project" and switcher has to be closed and opened again.

@mbien
Copy link
Member Author

mbien commented Nov 3, 2021

@KacerCZ ah interesting. Going to take a look at this case tomorrow. I might just add another commit which removes the async code. I could probably test it by putting a project on a usb drive or something similar.

Thanks for linking the issue to the PR.

@mbien
Copy link
Member Author

mbien commented Nov 4, 2021

ca3c620 has the async logic removed. Updated PR text.

@KacerCZ
Copy link
Contributor

KacerCZ commented Nov 4, 2021

I'm happy to confirm that after last commit the issue seems to be fixed.

@mbien mbien changed the base branch from master to delivery November 6, 2021 03:29
@mbien
Copy link
Member Author

mbien commented Nov 6, 2021

rebased onto delivery, since I saw there will be a RC 3

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Nov 8, 2021

@mbien I'm not sure about integrating this in to the 12.6 release, but also not investigated the changes in detail. Does this move the project resolution via FileOwnerQuery on to the EDT?

@neilcsmith-net neilcsmith-net added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Nov 8, 2021
@neilcsmith-net neilcsmith-net added this to the 12.6 milestone Nov 8, 2021
@mbien
Copy link
Member Author

mbien commented Nov 8, 2021

@mbien I'm not sure about integrating this in to the 12.6 release, but also not investigated the changes in detail. Does this move the project resolution via FileOwnerQuery on to the EDT?

yes. but at this point everything should be already in the cache. I had about 100 projects open and opened one file per project and the switcher appeared basically right away after IDE start.

If you look at the doc of FileOwnerQueryImplementation:

* Knowledge of which project some files belong to.
* <p>An implementation must attempt to return a result quickly and avoid
* blocking on foreign locks. In particular, it should not call {@code OpenProjects}.

If it is still determined that it still needs to be off EDT i would probably try to decouple it a few layers higher, e.g

private Item[] createSwitcherItems(final Controller controller) {

to avoid getting barraged by property change events fired from inner loops.

@neilcsmith-net
Copy link
Member

Yes, I read the docs and looked at a couple of the implementations. I'm not sure if there might be edge cases that could lock up the EDT or not here. I presume it was made async for a reason. I also agree that moving up a few layers would make more sense.

In terms of 12.6, going to request review - I'm not making the call on whether to merge, but it seems a slightly risky one to me at this stage when we're hoping this will be the final release.

@mbien
Copy link
Member Author

mbien commented Nov 8, 2021

no need to rush anything. I rebased most of my bugfix PRs just in case someone wants to integrate them while i am not available. Lets get 12.6 released ;)

@neilcsmith-net
Copy link
Member

OK, punting this to NB13 as no-one has reviewed.

@neilcsmith-net neilcsmith-net modified the milestones: 12.6, NB13 Nov 10, 2021
@neilcsmith-net neilcsmith-net changed the base branch from delivery to master November 10, 2021 12:15
@netbeansuser2019
Copy link

Great, finally you pick NETBEANS-3752 up.

@KacerCZ
Copy link
Contributor

KacerCZ commented Dec 11, 2021

I was using NetBeans with this fix daily for one week without any problems.

@mbien mbien added the kind:bug Bug report or fix label Dec 28, 2021
- file owner query is now a blocking opperation with timeout
- under normal conditions the query returns a result right away
- if it doesn't for whatever reason it won't block the EDT
- it fixes the issue as side effect + avoids repaint flickering
@mbien
Copy link
Member Author

mbien commented Jan 9, 2022

changed the impl as previously discussed to make it less risky. (decoupled from EDT on a higher level, one task for all tabs with timeout)

edit: for manual testing try to set a breakpoint inside the task, eg. there: https://github.com/apache/netbeans/pull/3299/files#diff-793eded4bbc5a3b7a16c8d300f53f467eb7538137d6d7338a36c433cf11473daR73 and then open the tab switcher.

@mbien mbien added Editor and removed do not merge Don't merge this PR, it is not ready or just demonstration purposes. labels Jan 9, 2022
@mbien
Copy link
Member Author

mbien commented Jan 17, 2022

@matthiasblaesing @neilcsmith-net @timboudreau any opinions on this. The code which unblocks the EDT is here

92619c9#diff-793eded4bbc5a3b7a16c8d300f53f467eb7538137d6d7338a36c433cf11473daR80

The task is usually done in <10ms. Fallback would be no groups in the popup on time out - but this does not happen under normal operation. This is purely there to ensure that the EDT can in fact not be blocked indefinitely, without having to review the entire implementation behind the project lookup logic for this minor fix (there is a lot of caching going on there).

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Jan 17, 2022

@mbien merging so we get it in 13-rc1. I would normally be inclined to trigger back to the EDT via invokeLater with something like this. But here, this should be fine, and I suspect with any open tabs the information is already calculated as you said earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editor kind:bug Bug report or fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants