make fragmentation tabs closeable by ToLeWeiss · Pull Request #214 · FelixBaensch/MORTAR

ToLeWeiss · 2026-01-21T13:56:53Z

This attempts to make the fragmentation tabs in MORTAR closeable and tries to clean up all data that is not used anymore as discussed in #80 because the fragments are also saved in the general MoleculeDataModel. As of my current knowledge there are two things to regard:

The molecules tab should not be closeable (it currently makes no sense to do so as we have only one molecule data set at a time and if one wants a new one it can just be imported which in turn deletes the old one)
the fragments can only be cleaned up if both the items tab and the fragments tab of the same fragmentation are closed because both hold references to the fragments

…ike not all the fragment data is cleaned up after both items and fragments tab are closed

…ce and the fragments map

ToLeWeiss · 2026-02-04T15:08:44Z

Make a global setting to turn of warnings about data loss if preferred and add a checkbox to the warnings about data loss or closing all tabs with "do not show this again" or similar.

…e a checkbox to turn off the warning message. To turn the messages back on there is also a global setting in preferences

…ommented out line

Copilot

Pull request overview

This pull request implements closeable fragmentation tabs in MORTAR, allowing users to close individual fragmentation result tabs (fragments and itemization tabs) or all tabs at once via context menus. The implementation includes data cleanup to free memory when tabs are closed and a user setting to control warning dialogs about data loss.

Changes:

Added a new user setting to control whether warnings are shown when data will be lost upon closing tabs or the application
Implemented tab closing functionality with context menus for individual and batch tab closure
Added cleanup logic to remove fragment data when both related tabs (fragments and itemization) for a fragmentation are closed

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
gradlew	Standard Gradle wrapper script addition
Message_en_GB.properties	Added warning messages for tab closure and new setting description (contains typo)
SettingsContainer.java	Added new setting for controlling data loss warnings
MoleculeDataModel.java	Added method to clear fragments for a specific fragmentation
FragmentDataModel.java	Made frequency fields final for improved immutability
GuiUtil.java	Added confirmation dialog with deactivation checkbox (has critical bug)
MainViewController.java	Implemented tab closing logic with cleanup and context menus (has multiple bugs)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/main/resources/de/unijena/cheminf/mortar/message/Message_en_GB.properties

Copilot · 2026-02-16T12:03:43Z

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

+            if (Objects.equals(tmpFragmentationName, ((GridTabForTableView) tmpTab).getFragmentationNameOutOfTitle())) {
+                return false;
+            }


The method isGridTabCleanable has a logical bug. It currently returns false if ANY tab with the same fragmentation name is found, including the tab being checked itself. This means isGridTabCleanable will always return false when checking a tab that's currently in the tab pane, preventing proper cleanup. The method should skip comparing the tab with itself by adding a check like: if (tmpTab == aGridTab) continue;

could this be the reason why we didn't see a substantial cleanup happening in VisualVM?

I tried it out but it does not seem to make a difference. In the methods of closeAllTabs and closeTab the tab is removed before the cleanup method is called so in these cases it should have been irrelevant (but bad nonetheless to have an implicit order of operations). Right now i can not tell why the setOnClosed method also seems to be unaffected by the additional check.

My best guess would be that the best effort of the gc request does not free the older objects when requesting generational gc (via the "Perform GC" button in visualvm) for the first time.
But this whole behaviour is quite strange as it seems that fragmenting and cleaning up a second time frees more memory then the first cleanup.

The picture below shows:

Action Used Heap (Mb)

1. Loading in the molecules and GC after 135

2. Fragmenting the molecules and GC after 254

3. Closing tabs by clicking the x button and GC after 187

4. Fragmenting again with GC after 255

5. Closing the tabs again with GC after 149

I recorded this behaviour with and without the above check.

I recorded this behaviour with and without the above check.

and did it change with/without the check?

For now, just investigate (e.g. by debugging) whether Copilot is right here and fix it if necessary. The GC behaviour would be a problem for afterwards.

It did not change with or without the check but I think it is definitely the right approach to check it. I already fixed it with the simple addition of the check but it did not really solve the GC mystery.

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

src/main/java/de/unijena/cheminf/mortar/gui/util/GuiUtil.java

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

src/main/java/de/unijena/cheminf/mortar/gui/util/GuiUtil.java

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

JonasSchaub

First part of review, almost done with MainViewController

src/main/java/de/unijena/cheminf/mortar/gui/util/GuiUtil.java

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

JonasSchaub · 2026-02-18T12:44:29Z

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

+            if (Objects.equals(tmpFragmentationName, ((GridTabForTableView) tmpTab).getFragmentationNameOutOfTitle())) {
+                return false;
+            }


could this be the reason why we didn't see a substantial cleanup happening in VisualVM?

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

JonasSchaub

Finished review, please look into the things I flagged up.

JonasSchaub · 2026-02-19T17:26:45Z

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

+
+        aGridTab.getProperties().clear();
+        aGridTab.setId(null);
+        aGridTab.setText(null);


Is all of this really necessary to free the objects for garbage collection? Maybe fix the problem above that (apparently), no tab is really identified as "cleanable" right now and then check again whether memory is actually free up with/without all this explicit null-setting.

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

src/main/java/de/unijena/cheminf/mortar/gui/util/GuiUtil.java

src/main/java/de/unijena/cheminf/mortar/model/settings/SettingsContainer.java

src/main/resources/de/unijena/cheminf/mortar/message/Message_en_GB.properties

…now especially the setOnDrag listener (confirmed via visualvm)

…he element itself but the fragments in the fragmentationService still remain

… really clear to me

…d removed them properly

… now cleans all generated fragment data models properly and added some documentation and comments

ToLeWeiss · 2026-03-09T13:53:05Z

From my point of view the cleanup looks pretty nice now!

The behaviour is like follows:

Loading in the first 928 molecules of the COCONUT and suggesting GC:

Fragmenting with the Ertl algorithm and suggesting GC:

Closing fragments and items tab and suggesting GC:

There is still a difference of about 2.5 MB but the fragment models get fully cleaned up.

The warning window with checkbox has now a different sizes which probably depends on the length of the title as the window of the main application upon trying to close, looks like:

which has less width than the same warning window called from the closing of tabs with a longer title:

Is the dependance on the length of the title plausible and would that be alright @JonasSchaub ?

Copilot

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-10T14:57:22Z

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

+        mainTabPane.getTabs().removeIf(tmpTab -> {
+            // it is assumed here that MORTAR does NOT have any other
+            // types of tabs that could be closed besides the GridTableView
+            // as the itemization tab and fragmentation tab are both constructed
+            // as GridTableViews
+            if (tmpTab.isClosable() && tmpTab instanceof GridTabForTableView tmpGridTab) {
+                this.cleanupGridTab(tmpGridTab);
+                return true;
+            }
+            return false;
+        });


closeAllTabs() calls cleanupGridTab(tmpGridTab) inside removeIf before the tabs are removed. cleanupGridTab() relies on isGridTabCleanable(), which checks for other tabs with the same fragmentation name. When closing all tabs, the paired tab (items/fragments) still exists at that moment, so isGridTabCleanable() returns false and the fragmentation data is never cleaned up even though both tabs are being closed. Consider a two-pass approach: first collect/remove all closable GridTabForTableView tabs, then run cleanup (or at least data cleanup) after the removals so the “no other tab exists” check reflects the final state.

Suggested change

mainTabPane.getTabs().removeIf(tmpTab -> {

// it is assumed here that MORTAR does NOT have any other

// types of tabs that could be closed besides the GridTableView

// as the itemization tab and fragmentation tab are both constructed

// as GridTableViews

if (tmpTab.isClosable() && tmpTab instanceof GridTabForTableView tmpGridTab) {

this.cleanupGridTab(tmpGridTab);

return true;

}

return false;

});

// First collect all closeable GridTabForTableView tabs

List<GridTabForTableView> tmpTabsToClose = mainTabPane.getTabs().stream()

// it is assumed here that MORTAR does NOT have any other

// types of tabs that could be closed besides the GridTableView

// as the itemization tab and fragmentation tab are both constructed

// as GridTableViews

.filter(tmpTab -> tmpTab.isClosable() && tmpTab instanceof GridTabForTableView)

.map(tmpTab -> (GridTabForTableView) tmpTab)

.collect(Collectors.toList());

// Remove them from the TabPane so that cleanup checks see the final state

mainTabPane.getTabs().removeAll(tmpTabsToClose);

// Now clean up the data for each removed tab

for (GridTabForTableView tmpGridTab : tmpTabsToClose) {

this.cleanupGridTab(tmpGridTab);

}

@ToLeWeiss, please have a look at the logic here.

@ToLeWeiss, I guess Copilot is wrong here? As I understand it, when the first tab of a pair should be removed, the data is not cleared (since the other tab still exists) but(!) the tab is removed nonetheless. So, when the second tab of the pair is set for removal, all should be cleared. Correct? You also confirmed this in VisualVM, as I understood it.

I was worried that removeIf possible does not remove objects eagerly but this does not seem to be the case but I will convert the functional removeIf to a more procedural approach to make it more readable:
https://github.com/openjdk/jdk/blob/8ecb14cecf1f05ea7c65fcfb5bb05e1e3a895d72/src/java.base/share/classes/java/util/Collection.java#L576-L587

I do not quite get the comment about the isGridTabCleanable as the function skips if it is the same tab that needs to be checked so this seems wrong to me:

MORTAR/src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

Lines 1456 to 1467 in e26bc04

private boolean isGridTabCleanable(GridTabForTableView aGridTab) {

String tmpFragmentationName = aGridTab.getFragmentationNameOutOfTitle();

for (Tab tmpTab : this.mainTabPane.getTabs()) {

if (!(tmpTab instanceof GridTabForTableView tmpGridTableTab) || tmpTab == aGridTab) {

continue;

}

if (tmpFragmentationName.equals(tmpGridTableTab.getFragmentationNameOutOfTitle())) {

return false;

}

}

return true;

}

src/main/java/de/unijena/cheminf/mortar/model/settings/SettingsContainer.java

Copilot · 2026-03-10T14:57:23Z

src/main/java/de/unijena/cheminf/mortar/model/fragmentation/FragmentationTask.java

    /**
     * Applies the IMoleculeFragmenter.fragment(IAtomContainer container) method on all given
-     * molecules and counts the occurring exceptions.
+     * molecules and records the exceptions and various statistics for the resulting fragments as a
+     * {{@link FragmentationTaskResult}}.
+     *
+     * @return a {{@link FragmentationTaskResult}} containing:
+     *          1. Nr. of exceptions that happened during molecule fragmentation.
+     *          2. Nr. of molecules that were successfully(!) fragmented but produced no fragments.
+     *          3. Nr. of molecules that were successfully(!) fragmented and produced fragments (to check whether all molecules are a
+     *          4. Nr. of molecules that gave no atom container.
+     *          5. Nr. of molecules that were filtered according to the respective fragmentation algorithm method.
+     *          6. Nr. of fragments that produced no SMILES code.
+     *          7. Nr. of unexpected exceptions that happened during molecule fragmentation (i.e. the whole process of preprocessing,
     *


The updated Javadoc uses invalid syntax and appears incomplete: "{{@link FragmentationTaskResult}}" should be "{@link FragmentationTaskResult}", and the numbered @return description is truncated (e.g., item 3 ends mid-sentence, item 7 is incomplete). This will break Javadoc generation and is confusing for readers; please correct the link markup and complete/format the return description.

@ToLeWeiss, I feel like this was actually my code edit, or did you add the doc?

This function had no documentation for the return type/value so I added some text and copied most of the documentation of the fragmentation result.

src/main/java/de/unijena/cheminf/mortar/model/fragmentation/FragmentationService.java

JonasSchaub · 2026-03-10T15:06:25Z

@ToLeWeiss, that looks great! The remaining 2.5 MB are negligible.

Is the dependance on the length of the title plausible and would that be alright @JonasSchaub ?

Sure, that would be alright, since all the dialogs scale according to their contents. What you could do here, of course, would be to turn the title of the first dialog to sth like "The app will be closed and data will be lost" to easily mitigate this here.

JonasSchaub · 2026-03-10T15:07:32Z

I'll have a closer look at the code tomorrow!

JonasSchaub

First part of review, mostly nitpicking.

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

JonasSchaub · 2026-03-11T13:26:12Z

src/main/java/de/unijena/cheminf/mortar/controller/MainViewController.java

    }
    //
+    /**
+     * Makes a menu item to close all tabs upon pressing and confirming a warning message.


It would be more precise to always specify fragmentation (result) tabs in this context, since the molecules tab remains. But well, maybe try having an AI correct this everywhere once. If it screws up, forget it; if it succeeds, good. Too much to ask to correct this manually everywhere now.

src/main/java/de/unijena/cheminf/mortar/gui/controls/GridTabForTableView.java

src/main/java/de/unijena/cheminf/mortar/gui/util/GuiUtil.java

JonasSchaub

Final part of review. I guess if you adjust everything accordingly and we discuss the one or two points that still need discussion, I do not need to review this again.

src/main/java/de/unijena/cheminf/mortar/model/fragmentation/FragmentationService.java

src/main/java/de/unijena/cheminf/mortar/model/settings/SettingsContainer.java

src/main/resources/de/unijena/cheminf/mortar/message/Message_en_GB.properties

JonasSchaub · 2026-03-12T10:30:41Z

@ToLeWeiss, I'm looking at the GUI functionality now. First point: after "close all" or if all fragmentation result tabs were closed, the main menu needs to be adjusted, i.e. the "File -> Export" menu and the "Views -> Histogram" menu item need to be disabled:
this.mainView.getMainMenuBar().getExportMenu().setDisable(true); this.mainView.getMainMenuBar().getHistogramViewerMenuItem().setDisable(true);

JonasSchaub · 2026-03-12T10:35:32Z

Related to that: if the items (or fragments) tab has been closed, the items (or fragments) data cannot be exported anymore, currently, even though the corresponding fragments (or items) tab still exists. MORTAR will show a notification saying "no data available for export". I guess this is fine; disabling the respective main menu item seems too complicated. But this behaviour needs to be noted somewhere, e.g. in the tutorial.

JonasSchaub · 2026-03-12T10:38:13Z

A different thing: if I close the fragments tab and then try to open the histogram view, I get an error. This must be corrected. I think the histogram already has a listener tracking which tab is active, since the menu item is disabled if the molecules tab is active. There, the case should be added that an items tab is active but has no corresponding fragments tab anymore and the menu item should also be disabled in that case.

…proach with less duplication

…roved the histogram opening logic, also introduced first try at fixing the export

sonarqubecloud · 2026-04-09T13:54:21Z

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

ToLeWeiss added 5 commits January 21, 2026 13:24

WIP: first draft of closeable tabs, but in the memory view it seems l…

7743405

…ike not all the fragment data is cleaned up after both items and fragments tab are closed

WIP: added additional final declarations

cbbaced

WIP: small renaming and clearing the cache of the fragmentation servi…

7b600b0

…ce and the fragments map

added menu items to close one or all tabs and refactored the code

7b69374

removed unused imports

b0d6aae

ToLeWeiss added 2 commits February 12, 2026 15:50

added a gui utility and changed the occurences of data loss to includ…

957626f

…e a checkbox to turn off the warning message. To turn the messages back on there is also a global setting in preferences

calming sonar cloud by reducing cognitive complexity and removing a c…

51500bd

…ommented out line

ToLeWeiss requested a review from JonasSchaub February 12, 2026 15:26

JonasSchaub requested a review from Copilot February 16, 2026 11:57

Copilot started reviewing on behalf of JonasSchaub February 16, 2026 11:57 View session

Copilot AI reviewed Feb 16, 2026

View reviewed changes

JonasSchaub marked this pull request as ready for review February 16, 2026 12:51

JonasSchaub requested changes Feb 18, 2026

View reviewed changes

JonasSchaub requested changes Feb 19, 2026

View reviewed changes

ToLeWeiss added 12 commits February 20, 2026 18:10

wip: partial application of the fixes

db452ea

wip: further implementation of the fixes

f8b1748

finishing the implementation of the fixes

54500eb

generalising utility type documentation

97a602d

wip: properly remove all listeners that still hold references. Right …

fbe2e74

…now especially the setOnDrag listener (confirmed via visualvm)

wip: added missing documentation

ff6d166

wip: small improvements to documentation and moved cleanup of UI to t…

32b909d

…he element itself but the fragments in the fragmentationService still remain

wip: added two FIXME's with comments becuase these two things are not…

d043395

… really clear to me

wip: applied spotless

22c6282

wip: added documentation for the fragments in fragmentationService an…

9c97e5e

…d removed them properly

properly removes fragments in the fragmentation service and therefore…

1cd36ad

… now cleans all generated fragment data models properly and added some documentation and comments

removing FIXME comment

e26bc04

JonasSchaub requested a review from Copilot March 10, 2026 14:50

Copilot started reviewing on behalf of JonasSchaub March 10, 2026 14:51 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

JonasSchaub requested changes Mar 11, 2026

View reviewed changes

JonasSchaub requested changes Mar 12, 2026

View reviewed changes

ToLeWeiss added 5 commits March 16, 2026 18:56

wip: applied first part of the fixes

7bd1624

added todo to remember

221a9d8

applied fixes and refactored the GuiUtil alerts for a more generic ap…

c72a578

…proach with less duplication

fixed the todo comment

d7d66d4

wip: fixed the falsely displayed export and histogram buttons and imp…

40bf0e5

…roved the histogram opening logic, also introduced first try at fixing the export

Action	Used Heap (Mb)
1. Loading in the molecules and GC after	135
2. Fragmenting the molecules and GC after	254
3. Closing tabs by clicking the x button and GC after	187
4. Fragmenting again with GC after	255
5. Closing the tabs again with GC after	149

	private boolean isGridTabCleanable(GridTabForTableView aGridTab) {
	String tmpFragmentationName = aGridTab.getFragmentationNameOutOfTitle();
	for (Tab tmpTab : this.mainTabPane.getTabs()) {
	if (!(tmpTab instanceof GridTabForTableView tmpGridTableTab) \|\| tmpTab == aGridTab) {
	continue;
	}
	if (tmpFragmentationName.equals(tmpGridTableTab.getFragmentationNameOutOfTitle())) {
	return false;
	}
	}
	return true;
	}

Conversation

ToLeWeiss commented Jan 21, 2026

Uh oh!

ToLeWeiss commented Feb 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JonasSchaub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JonasSchaub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ToLeWeiss commented Mar 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment