CSSTUDIO-1997 Add option to open most recently focused tab when closing a tab

[abrahamwolk]

This merge-request adds the option org.phoebus.ui/open_previous_tab_when_closing_tab to Phoebus. When org.phoebus.ui/open_previous_tab_when_closing_tab=true is set (the default is false), then Phoebus will set the focus on the most recently focused tab when closing the active tab. If a tab is closed that is not active, the focus remains unchanged.

[shroffk]

This is quite intuitive, I would even be in favour of making the default true.

[kasemir]

I would even be in favour of making the default true.

I'd go even further: Make that THE behavior. No added preference, that's just how it'll be.

For the implementation, I'm worried that List<DockItem> tabsInOrderOfFocus grows with reference to tabs that no longer exist, creating a memory leak. Maybe change it to a list of weak references?

[abrahamwolk]

For the implementation, I'm worried that List<DockItem> tabsInOrderOfFocus grows with reference to tabs that no longer exist, creating a memory leak. Maybe change it to a list of weak references?

Can tabs be closed without invoking the event handler set by setOnCloseRequest() (which will remove the DockItem from tabsInOrderOfFocus)?

[abrahamwolk]

I have now made the behavior the only behavior.

[kasemir]

setOnCloseRequest() ..will remove the DockItem from tabsInOrderOfFocus

I'm not 100% sure: Is that only called when clicking the 'x' on the tab? Or also when closing the window?

Regarding setOnCloseRequest, you add that call in DockItem line ~179, but there's also a call around line 597 for addCloseCheck which will now be disabled because getOnCloseRequest() != null:

    public void addCloseCheck(final Supplier<Future<Boolean>> ok_to_close)
    {
        if (getOnCloseRequest() == null)
            setOnCloseRequest(event ->
            {

[abrahamwolk]

setOnCloseRequest() ..will remove the DockItem from tabsInOrderOfFocus

I'm not 100% sure: Is that only called when clicking the 'x' on the tab? Or also when closing the window?

You are right that the OnCloseRequest event-handler is not called when closing the window. I have added the following code to handleClosed() which is called when the window is closed:

var dockPane = getDockPane();
if (dockPane != null) {
    dockPane.tabsInOrderOfFocus.remove(this);
}

However, isn't the whole DockPane instance de-allocated when a window is closed? It is good to have redundancy, however. And perhaps a tab can be closed in other ways, also, without invoking the OnCloseRequest event-handler.

Regarding setOnCloseRequest, you add that call in DockItem line ~179, but there's also a call around line 597 for addCloseCheck which will now be disabled because getOnCloseRequest() != null:

    public void addCloseCheck(final Supplier<Future<Boolean>> ok_to_close)
    {
        if (getOnCloseRequest() == null)
            setOnCloseRequest(event ->
            {

That is a good point! In the PR #2758 (CSSTUDIO-1987 New "Unsaved Changes" confirmation dialog) I have changed this behavior: instead the close-request handler is installed unconditionally in the constructor of DockItem. The two handlers in the two PRs will have to be combined. When I have merged one of these PRs, I will update the other so that the two handlers are combined into one.

For the time being, I have now modified addCloseCheck() in this PR to call any event handler that was already installed. setOnCloseRequest() is now invoked by addCloseCheck() as follows:

var alreadyExistingEventHandler = getOnCloseRequest();

setOnCloseRequest(event -> {
    // For now, prevent closing
    event.consume();
    // Invoke all the ok-to-close checks in background threads
    // since those that save files might take time.
    JobManager.schedule("Close " + getLabel(), monitor ->
    {
        if (prepareToClose()) {
            if (alreadyExistingEventHandler != null) {
                alreadyExistingEventHandler.handle(event);
            }
            Platform.runLater(() -> close());
        }
    });
});

I propose that this temporary fix be merged in this PR, and that I then combine the two event-handlers into one event-handler that is installed unconditionally in the constructor of DockItem in #2758.

[kasemir]

I propose that this temporary fix be merged in this PR, and that I then combine the two event-handlers into one event-handler

OK, let's merge this one first, then you update the other and test it for a little while, then we merge the other.

[abrahamwolk]

OK, let's merge this one first, then you update the other and test it for a little while, then we merge the other.

That sounds like a plan!

[abrahamwolk]

With the new fixes in place, are there still concerns about memory leakages? And are there any other concerns? Or can I go ahead and merge this version?

[kasemir]

Go ahead