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

feat(gui): Quick Tabs Overhaul #2241

Merged
merged 16 commits into from
Aug 6, 2024
Merged

Conversation

Mino260806
Copy link
Contributor

@Mino260806 Mino260806 commented Aug 5, 2024

  • (internal) Decentralize Tabs Controlling code to a seperate TabsController class. (*1)
  • Add Open Tabs parent node in Quick Tabs that allows users to view tabs that are open
  • Add Bookmarked Tabs parent node in Quick Tabs that allows users to bookmark tabs and keep a reference even when the tab is closed

Unchecked tasks are WIP


(*1) This change is a must. Previously, TabbedPane was the only class interacting with tabs. So it was fine that all the code that controls tabs was found there. However now, that we are adding QuickTabsTree, (and why not other components), it no longer makes sense that QuickTabsTree needs to communicate with TabbedPane in order to manage the tabs. Counter example ? There may be tabs which are not open in TabbedPane but are present in QuickTabsTree, like bookmarked tabs. So if we would keep using old code, we'd have to store in TabbedPane a seperate reference to tabs that are present but not open, which would transform the code into a mess. I hope you understood the point.

Now TabsController hold a "generic reference" to each tab, and TabbedPane holds a "component reference".

Before

image

Now

image


Affected issues

Icons from https://intellij-icons.jetbrains.design

@Mino260806 Mino260806 marked this pull request as ready for review August 6, 2024 00:47
Copy link
Owner

@skylot skylot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I found some issues and request small improvements 🙂

@skylot
Copy link
Owner

skylot commented Aug 6, 2024

One more thing.
I notice that on reload (or close jadx-gui and open project again) tabs order changes.
To reproduce, move any tab to other position and press reload action (from toolbar or F5) and tabs will reorder.
Not sure, maybe that was broken before this commit, but as soon as we change saving code here it is better to use actual tabs order on project save.

@Mino260806 Mino260806 requested a review from skylot August 6, 2024 21:21
@Mino260806
Copy link
Contributor Author

@skylot all ready

@skylot
Copy link
Owner

skylot commented Aug 6, 2024

Hm, not sure why we need to save index to restore order, is just saving/reading tabs list in correct order not working?

@Mino260806
Copy link
Contributor Author

Hm, not sure why we need to save index to restore order, is just saving/reading tabs list in correct order not working?

TabsController now holds the generic reference to each TabBlueprint through a Map<JNode, TabBlueprint>, I avoided saving the tabs in the TabbedPane order because

  • it would require a callback to TabbedPane before saving tabs that reorders TabBlueprint and saves them back, and we would need filtering for tabs that are hidden
  • we would anyway use the same procedure (store index) if we wanted to also preserve order for QuickTabs and allow reordering later on

But maybe we can just consider the order of TabbedPane to be the "default" order, ok I'll implement it by saving/reading tabs list in correct order

@Mino260806
Copy link
Contributor Author

@skylot please see commit 6ca79d4. I implemented saving/reading tabs list in correct order

@skylot
Copy link
Owner

skylot commented Aug 6, 2024

OK, I think we can merge it now.
Although, here is a small list of thing for future improvements:

  • There are still a lot of calls using TabbedPane instead TabsController. Like in MainWindow we call method in TabbedPane next it calls TabsController, and it calls TabbedPane again 🤣. This is confusing and not follow architecture you draw 😭
  • It feels like onTabsReorder method implementation has very high complexity (like 2*n^2) and can take a lot of time for lots of tabs and bookmarks, so it is better to optimize it. And in general feels like that we are very often iterating over lists in search of one element, maybe some additional map will help.
  • Please do not use ArrayList as a variable type, it is considered a bad practice in Java. I think checkstyle has rule for that (this), I will enable it and fix all such places.

@skylot skylot merged commit ffbf800 into skylot:master Aug 6, 2024
5 checks passed
@Mino260806
Copy link
Contributor Author

Although, here is a small list of thing for future improvements

All noted. I'll open a new PR in the near future

@Mino260806 Mino260806 deleted the feat_quick_tabs branch August 7, 2024 10:13
@Mino260806 Mino260806 mentioned this pull request Aug 7, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Bookmarks and search history [feature] Add the ability to bookmark a node for later reference
2 participants