Bug 235197 - [api] New Connection Wizard gets unusable after cancelling on the first page
Summary: [api] New Connection Wizard gets unusable after cancelling on the first page
Status: RESOLVED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 3.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.0 RC3   Edit
Assignee: Martin Oberhuber CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords: api
: 235443 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-06-02 14:27 EDT by Martin Oberhuber CLA
Modified: 2008-06-03 16:01 EDT (History)
3 users (show)

See Also:
mober.at+eclipse: pmc_approved+
uwe.st: review+


Attachments
Patch fixing the issue (41.21 KB, patch)
2008-06-02 16:08 EDT, Martin Oberhuber CLA
no flags Details | Diff
Updated patch after committing bug 235148 (36.71 KB, patch)
2008-06-03 04:53 EDT, Martin Oberhuber CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Oberhuber CLA 2008-06-02 14:27:03 EDT
CQ:WIND00124513

* Select an existing connection in RSE, e.g. "Local"
* Press New Connection button
* Press Cancel immediately on the first wizard page (system type selection)
  -- Optionally: click on any system type to select it before canceling
* Press New Connection button again
* (after selecting same system type), press Next button

--> RSE Wizard says "Connection already exists" and next button is disabled
--> There is no chance creating the connection

Workaround: 
Cancel the wizard, then press "New Connection" again, this time it works

-----------Enter bugs above this line-----------
TM 3.0RC2 testing
installation : eclipse-SDK-3.4RC3 (I20080530-1730), cdt-5.0RC2 (200805231539), 
     DSF-1.0rc2 (20080527), emf-2.4.0rc2, Findbugs-1.3.3, Releng.Tools-3.4RC3, 
     RSE-3.0RC2, RXTX-2.1-7r3b, Subversive-0.7.0.v20080517, FTP_WebDav-3.2.2
     org.tigris.MemMonitor, WR-Retriever-3.0.v20070604,
RSE install  : RSE 3.0RC2 in workspace, TM-terminal, TM-discovery
java.runtime : Sun 1.6.0_05-b13 -Xmx512m -XX:MaxPermSize=128m
os.name:     : Windows XP 5.1, Service Pack 2
------------------------------------------------
systemtype   : Windows-local, Dstore-win, Dstore-linux
targetos     : Red Hat Enterprise Linux WS release 4 (Nahant Update 3)
targetuname  : Linux parser 2.6.9-34.EL #1 i686 athlon i386 GNU/Linux
targetvm     : Sun Java HotSpot(TM) Client VM (build 1.4.2_12-b03, mixed mode)
------------------------------------------------
Comment 1 Martin Oberhuber CLA 2008-06-02 16:08:52 EDT
Created attachment 103196 [details]
Patch fixing the issue

The problem is, that the wizard selection page already instantiates and initializes the selected wizard in order to know whether nextPage() is possible, and this calls addPages() in the wizard. 

When pressing cancel, the already-initialized wizard is kept alive in the RSENewConnectionWizardRegistry, which is a Singleton, and on the next invocation addPages() is called again: that way, the first page of a wizard exists twice. The later instance is edited by the user; but the older instance is used for verifications and getting data, so the user cannot edit the data any more.

Attached patch fixes the issue, by using a new RSENewConnectionWizardRegistry instance for each wizard invocation. That way, wizard instances are re-used as long as the wizard is open; but when the wizard closes, they are removed (garbage collected).

This is a backward compatible API change, but I thought it's better changing API than trying to work around the wizard lifetime issues with any obscure hacks. 
Having the RSENewConnectionWizardRegistry be a Singleton was a problematic decision from the beginning, so the corresponding getInstance() call is now deprecated. Clients should create their own wizard registry instances, such that they can control their lifetime - which should be the lifetime of the corresponding wizard.

The solution therefore changes / adds the API in the following places:

RSENewConnectionWizardRegistry no longer @noinstantiate
RSENewConnectionWizardRegistry.getInstance() deprecated
RSENewConnectionWizardRegistry() Constructor now public

RSENewConnectionWizardSelectionPage(RSENewConnectionWizardRegistry)
RSENewConnectionWizardSelectionTreeDataManager(RSENewConnectionWizardRegistry)
RSEAbstractWizardSelectionTreeDataManager(RSEAbstractWizardRegistry)

The change is backward compatible, since the old constructors are still there (but deprecated). For the sake of system robustness, however, I'd recommend making the entire RSEMainNewConnectionWizard (wizard selection implementation) inernal non-API. I have filed bug 235202 for this (API breaking) request. Note: the patch also includes the fix for bug 235148 (dead code removal).
Comment 2 Martin Oberhuber CLA 2008-06-02 16:09:54 EDT
Uwe please review this fix for 3.0RC3 inclusion. 
Comment 3 Martin Oberhuber CLA 2008-06-03 04:53:49 EDT
Created attachment 103263 [details]
Updated patch after committing bug 235148
Comment 4 Martin Oberhuber CLA 2008-06-03 05:06:29 EDT
Patch committed:
[235197][api] Unusable wizard after cancelling on first page
Comment 5 David Dykstal CLA 2008-06-03 08:49:13 EDT
+1 Seems reasonable.
Comment 6 Martin Oberhuber CLA 2008-06-03 16:01:35 EDT
*** Bug 235443 has been marked as a duplicate of this bug. ***