From d4f190fecf6d721039def3174871071f30b74b02 Mon Sep 17 00:00:00 2001 From: Mark Taylor Date: Thu, 21 Aug 2014 16:50:42 +0100 Subject: [PATCH] topcat: fix regression bug (auth failure) in JDBC load dialogue When I added the TAP load dialogue to the ControlWindow toolbar just before release of TOPCAT v4.2 (4289ffb4), I inadvertently introduced a bug which meant that the SQL load dialogue doesn't work any more - authentication fails. I haven't tracked down exactly why this happens, but it's related to the fact that the LoadWindow was being instantiated during ControlWindow construction (before that change it was not done until the user hit the Load button). Modified ControlWindow so that LoadWindow is not constructed until it's needed, which fixes that bug. It's possible there were other unnoticed negative consequences of the earlier change; this update may fix those too. Both the problem and the fix are fairly inelegant, because application startup in topcat (implemented in the very old classes Driver, ControlWindow, LoadWindow) is messy and badly designed. Added a note in ControlWindow to this effect. --- topcat/src/docs/sun253.xml | 2 + .../uk/ac/starlink/topcat/ControlWindow.java | 57 ++++++++++++++++++- .../uk/ac/starlink/topcat/LoadWindow.java | 1 - 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/topcat/src/docs/sun253.xml b/topcat/src/docs/sun253.xml index 614258476e..85ec0acb97 100644 --- a/topcat/src/docs/sun253.xml +++ b/topcat/src/docs/sun253.xml @@ -20384,6 +20384,8 @@ introduced since the last version: http://reg.g-vo.org/tap, which should have good reliability, since it can point to different RegTAP services as required. +
  • Fix regression bug (introduced at v4.2) which caused authentication + to fail when using the SQL Query load dialogue.

    • diff --git a/topcat/src/main/uk/ac/starlink/topcat/ControlWindow.java b/topcat/src/main/uk/ac/starlink/topcat/ControlWindow.java index 1ecc88b2f6..af6585b403 100644 --- a/topcat/src/main/uk/ac/starlink/topcat/ControlWindow.java +++ b/topcat/src/main/uk/ac/starlink/topcat/ControlWindow.java @@ -145,6 +145,13 @@ * Main window providing user control of the TOPCAT application. * This is a singleton class. * + *

    Note: there is a lot wrong with this class. + * It's been here for as long as topcat has (i.e. since before I knew + * better), and it does far too much, often in the wrong way. + * It would be nice to do something about it one day, but in the + * meantime, don't assume that there's a good reason for all the + * implementation details that you see here. + * * @author Mark Taylor (Starlink) * @since 9 Mar 2004 */ @@ -478,8 +485,15 @@ public void stateChanged( ChangeEvent evt ) { "five existing tables", 5 ), }; + /* Hack. We want to get an action to launch the TAP load + * dialogue here. One way is to call getLoadWindow() and get it + * from there. However, that instantiates the Load Window too + * early, which is not only inefficient (not otherwise required + * at startup) but also causes some trouble with JDBC + * (the SQL load dialog fails for reasons I haven't identified). + * So do it another way. */ Action tapAct = - getLoadWindow().getDialogAction( TopcatTapTableLoadDialog.class ); + createLoadDialogAction( TopcatTapTableLoadDialog.class ); Transmitter tableTransmitter = communicator_ == null ? null @@ -578,7 +592,9 @@ public void mouseClicked( MouseEvent evt ) { /* Add join/match control buttons to the toolbar. */ toolBar.add( matchActs_[ 1 ] ); - toolBar.add( tapAct ); + if ( tapAct != null ) { + toolBar.add( tapAct ); + } toolBar.add( cdsmatchAct_ ); toolBar.addSeparator(); @@ -1930,6 +1946,43 @@ private static String shorten( String label ) { return label; } + /** + * Returns an action which will launch a load dialogue of a particular + * class. These individual load dialogues are really owned by the + * LoadWindow, which doesn't (and shouldn't) exist during + * ControlWindow construction, so we have to jump through some hoops. + */ + private Action + createLoadDialogAction( final Class + tldClazz ) { + + /* Get a dialogue instance for the metadata: name, icon, description. + * Throw it away after that. Not ideal. */ + TableLoadDialog tld0; + try { + tld0 = tldClazz.newInstance(); + } + catch ( Throwable e ) { + logger_.log( Level.WARNING, "Can't set up TAP action", e ); + return null; + } + + /* Construct and return an action which will lazily acquire the + * LoadWindow's copy of the relevant dialogue as required. */ + return new BasicAction( tld0.getName(), tld0.getIcon(), + tld0.getDescription() ) { + Action act_; + public void actionPerformed( ActionEvent evt ) { + if ( act_ == null ) { + act_ = getLoadWindow().getDialogAction( tldClazz ); + } + if ( act_ != null ) { + act_.actionPerformed( evt ); + } + } + }; + } + /** * Actions which correspond to output of a table. */ diff --git a/topcat/src/main/uk/ac/starlink/topcat/LoadWindow.java b/topcat/src/main/uk/ac/starlink/topcat/LoadWindow.java index c29b76f70e..d1e03cd0aa 100644 --- a/topcat/src/main/uk/ac/starlink/topcat/LoadWindow.java +++ b/topcat/src/main/uk/ac/starlink/topcat/LoadWindow.java @@ -284,7 +284,6 @@ public Action getDialogAction( Class tldClazz ) { } } } - assert false; return null; }