AAVSO / VStar

VStar is a visualisation and analysis tool for variable star data brought to you by AAVSO
https://www.aavso.org/vstar
GNU Affero General Public License v3.0
9 stars 3 forks source link

Period analysis plugin dialogs share period selection notification messages. #381

Closed mpyat2 closed 7 months ago

mpyat2 commented 8 months ago

If the user selects a new period in any period analysis dialog (DC DFT, WWZ), the notification message propagates through all open period analysis dialogs. To reproduce: do DC DFT, then do not close the dialog and do WWZ. Then, try to select a period in the table or in the plot. The selection in the other dialog will change. It's confusing.

Possible solutions: 1) add a dialog identifier to the message and process only the messages that relate to the specific dialog instance 2) a simpler solution: allow one and only period analysis dialog to be opened at the same time: add private static PeriodAnalysisDialogBase prevInstance to PeriodAnalysisDialogBase add the following code to the constructor at the beginning:

        if (prevInstance != null) {
            if (prevInstance.isVisible()) {
                prevInstance.setVisible(false);
                prevInstance.cleanup();
                prevInstance.dispose();
            }
        }
        prevInstance = this;
dbenn commented 8 months ago

I have not hit this problem because I don't think I've ever had two PA dialogs open at the same time.

So, solution 2 would close the first dialog, right? That may or may not be what the user expects I suppose. A variant would be to open a message dialog that says: only one PA at a time permitted.

Solution 1 is a good idea. It's not normally the way the Observer pattern works of course but it is somewhat like protocols such as MODBUS.

There may be another solution relating to message listener lifetimes and window focus. That needs a closer look though.

orionlee commented 8 months ago

One data point: When I use VStar (albeit only occasionally), I often run a DC DFT, followed by an AoV, and would keep the 2 dialogs open:

image

mpyat2 commented 8 months ago

... which makes the 2nd totalitarian-style solution inappropriate.

orionlee commented 8 months ago

BTW, the issue seems to be related to the old #308 .

mpyat2 commented 8 months ago

A side finding: sometimes, too many "PeriodAnalysisSelectionMessage" messages are generated. In PeriodAnalysis2DResultDialog: if the user clicks a maximum on the 2D chart, PeriodAnalysisSelectionMessage generates. This message is listened to by PeriodAnalysisTopHitsTablePane and PeriodAnalysisDataTablePane. In response to the message, setRowSelectionInterval() may be called. This call causes the valueChanged() event, which also generates PeriodAnalysisSelectionMessage. I think valueChanged() should be disabled while calling setRowSelectionInterval().

mpyat2 commented 8 months ago

Returning to the main topic: each component has a unique name; in particular, the period analysis dialogs have names like 'dialogXX'. This name may be attached to the message (for example, we can add a field 'name' (with getter/setter) to MessageBase). While responding to the message, the listener can check if the message name and the dialog name are the same. A name of the dialog can be found with a code like this:

                JDialog parentDialog = null;
                Component comp = this;
                while (comp != null) {
                    if (comp instanceof JDialog) {
                        parentDialog = (JDialog)comp;
                    }
                    comp = comp.getParent();
                }
mpyat2 commented 8 months ago

I've created a branch 'period-analysis-messages' which addresses this and #382 issues. It is not yet thoroughly tested, so the pull request will be created later.