Hide tabs without restart by koppor · Pull Request #12958 · JabRef/jabref

koppor · 2025-04-17T23:15:39Z

Follow-up to #12919

Hides tabs without reload.

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
[/] Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
[/] Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
[/] Screenshots added in PR description (if change is visible to the user)
[/] Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
[/] Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

trag-bot · 2025-04-17T23:16:47Z

                                  TaskExecutor taskExecutor
    ) {
-        super(aiService, aiPreferences, externalApplicationsPreferences, dialogService);
+        super(aiService, aiPreferences, externalApplicationsPreferences, dialogService, adaptVisibleTabs);


The constructor call in the superclass has been modified to include 'adaptVisibleTabs', but the JavaDoc for the constructor has not been updated to reflect this change.

trag-bot · 2025-04-17T23:17:26Z

+    private final AdaptVisibleTabs adaptVisibleTabs;

-    public AiPrivacyNoticeGuardedComponent(AiPreferences aiPreferences, ExternalApplicationsPreferences externalApplicationsPreferences, DialogService dialogService) {
+    public AiPrivacyNoticeGuardedComponent(AiPreferences aiPreferences, ExternalApplicationsPreferences externalApplicationsPreferences, DialogService dialogService, AdaptVisibleTabs adaptVisibleTabs) {


The JavaDoc for the constructor should be updated to include the new parameter 'AdaptVisibleTabs adaptVisibleTabs' as per the special instructions.

trag-bot · 2025-04-17T23:18:08Z

+                            AdaptVisibleTabs adaptVisibleTabs
    ) {
-        super(aiPreferences, externalApplicationsPreferences, dialogService);
+        super(aiPreferences, externalApplicationsPreferences, dialogService, adaptVisibleTabs);


The constructor call now includes an additional parameter 'adaptVisibleTabs', but there is no JavaDoc update reflecting this change. JavaDoc should be updated to include this new parameter.

trag-bot · 2025-04-17T23:18:32Z

+                                          AdaptVisibleTabs adaptVisibleTabs
    ) {
-        super(aiPreferences, externalApplicationsPreferences, dialogService);
+        super(aiPreferences, externalApplicationsPreferences, dialogService, adaptVisibleTabs);


The constructor call to the superclass now includes a new parameter 'adaptVisibleTabs', but the JavaDoc for the constructor has not been updated to reflect this change.

trag-bot · 2025-04-17T23:19:43Z

    private final AiService aiService;
    private final DialogService dialogService;
    private final StateManager stateManager;
+    private final AdaptVisibleTabs adaptVisibleTabs;


The field 'adaptVisibleTabs' is introduced but not documented in the JavaDoc of the constructor. This violates the requirement to update JavaDoc when method arguments change.

trag-bot · 2025-04-17T23:19:58Z

 * The editors for fields are created via {@link org.jabref.gui.fieldeditors.FieldEditors}.
 */
-public class EntryEditor extends BorderPane implements PreviewControls {
+public class EntryEditor extends BorderPane implements PreviewControls, AdaptVisibleTabs {


The class now implements AdaptVisibleTabs, but the JavaDoc for the class does not reflect this change. The JavaDoc should be updated to include information about the new interface implementation.

trag-bot · 2025-04-17T23:20:23Z

+                // Actions are recreated here since this avoids passing more parameters and the amount of additional memory consumption is neglegtable.
+                new UndoAction(this::getCurrentLibraryTab, undoManager, dialogService, stateManager),
+                new RedoAction(this::getCurrentLibraryTab, undoManager, dialogService, stateManager));
+        Injector.setModelOrService(EntryEditor.class, entryEditor);


The Injector.setModelOrService call is duplicated in the patch. This could lead to unnecessary redundancy and potential maintenance issues.

trag-bot · 2025-04-17T23:21:13Z

        this.dialogService = Objects.requireNonNull(dialogService);
        this.aiService = Objects.requireNonNull(aiService);
        this.preferences = Objects.requireNonNull(preferences);
+        this.adaptVisibleTabs = adaptVisibleTabs;


The use of Objects.requireNonNull should be replaced with @nonnull annotation to ensure the adaptVisibleTabs parameter is not null, aligning with the project's guidelines.

trag-bot · 2025-04-17T23:21:59Z

        this.dialogService = dialogService;
        this.aiService = aiService;
        this.stateManager = stateManager;
+        this.adaptVisibleTabs = adaptVisibleTabs;


The assignment of adaptVisibleTabs in the constructor lacks a null check, which violates the fail-fast principle by not immediately handling invalid states.

calixtus · 2025-04-18T18:13:47Z

I believe to correct future approach would be to observe an observable list of visible tabs, A Pane in the ai package would implement an interface of the EntryEditor interface and that could be added to or removed from the list.

koppor · 2025-04-18T18:30:10Z

I believe to correct future approach would be to observe an observable list of visible tabs, A Pane in the ai package would implement an interface of the EntryEditor interface and that could be added to or removed from the list.

Follow-up?

Still not clear how to access this list from "somewhere".

I was about to add an event bus for the general (cause sender and receiver are decoupled), but did not dare 😅

calixtus · 2025-04-18T18:44:17Z

Yes follow up eventually. When we rewrite the entry editor. List would sit in the StateManager.

Siedlerchr · 2025-04-21T11:39:54Z

Yeah, looks a bit weird now. I would have expected an observabelist of tabs in the state manager

koppor · 2025-04-21T13:28:37Z

Yeah, looks a bit weird now. I would have expected an observabelist of tabs in the state manager

That would have been much more effort for me 😅.

I did not use any dependency injection; instead explicitly passing the dependency down.

Proposal: Either someone improves the code significantly the next two weeks or we merge and do follow-up PRs.

Currently, restarting only because of hiding is bad UX IMHO.

calixtus · 2025-04-21T14:14:04Z

Im ok with follow-up. We need to push forward here. Will also increase pressure on us to finally work on the issues with the entry editor.

calixtus · 2025-04-21T14:14:32Z

Maybe put a fixme or todo in the code as a bad conscience reminder

calixtus · 2025-04-21T19:51:49Z

This strategy might also solve #12865

trag-bot · 2025-05-09T06:38:24Z

@@ -0,0 +1,11 @@
+package org.jabref.gui.entryeditor;
+
+/// This interface is used to adapt the visible tabs in the entry editor based on updated preferences


The comment is trivial as it restates the purpose of the interface, which is already clear from the interface name and method signature. Comments should provide additional insights or reasoning.

trag-bot · 2025-05-09T07:05:03Z

@trag-bot didn't find any issues in the code! ✅✨

github-actions · 2025-05-09T22:11:02Z

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

koppor added 2 commits April 18, 2025 00:54

Fix link

6543af0

Introduce interface AdaptVisibleTabs to avoid reload

d5b3ecd

koppor added the component: entry-editor label Apr 17, 2025

koppor mentioned this pull request Apr 17, 2025

Reorder settings in the Entry editor tab in preferences and buttons to hide AI tabs #12919

Merged

7 tasks

trag-bot Bot reviewed Apr 17, 2025

View reviewed changes

koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 18, 2025

koppor requested review from Siedlerchr and calixtus April 18, 2025 18:00

koppor added 2 commits May 9, 2025 08:21

Merge branch 'main' into avoid-restart

b371853

Add JavaDoc comment for TODO

8386d98

trag-bot Bot reviewed May 9, 2025

View reviewed changes

Compile fix

cf0e316

koppor mentioned this pull request May 9, 2025

"Search results" tab should be shown during fulltext search #12865

Closed

Siedlerchr approved these changes May 9, 2025

View reviewed changes

Siedlerchr enabled auto-merge May 9, 2025 21:28

Siedlerchr added this pull request to the merge queue May 9, 2025

Merged via the queue into main with commit f019d37 May 9, 2025
2 checks passed

Siedlerchr deleted the avoid-restart branch May 9, 2025 22:27

		@@ -0,0 +1,11 @@
		package org.jabref.gui.entryeditor;

		/// This interface is used to adapt the visible tabs in the entry editor based on updated preferences

Uh oh!

Conversation

koppor commented Apr 17, 2025

Mandatory checks

Uh oh!

trag-bot Bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

calixtus commented Apr 18, 2025

Uh oh!

koppor commented Apr 18, 2025

Uh oh!

calixtus commented Apr 18, 2025

Uh oh!

Siedlerchr commented Apr 21, 2025

Uh oh!

koppor commented Apr 21, 2025

Uh oh!

calixtus commented Apr 21, 2025

Uh oh!

calixtus commented Apr 21, 2025

Uh oh!

calixtus commented Apr 21, 2025

Uh oh!

trag-bot Bot May 9, 2025

Choose a reason for hiding this comment

Uh oh!

trag-bot Bot commented May 9, 2025

Uh oh!

github-actions Bot commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented May 9, 2025 •

edited

Loading