Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide error message if unsupported protocol was used #113

Merged
merged 6 commits into from
Jul 1, 2020

Conversation

dominikl
Copy link
Member

@dominikl dominikl commented Mar 12, 2020

See Gateway PR ome/omero-gateway-java#34 , respectively issue ome/omero-gateway-java#33 .

Test
Check that 'normal' login (hostname) and websocket login still works.
Check that you get a sensible error message if you try something like 'smb://someserver.org' .

@dominikl
Copy link
Member Author

Actually, in Insight I should not just pass on the error message, but use something more helpful for the user. I'll change that.

@@ -62,6 +62,7 @@
import javax.swing.event.DocumentListener;

import org.openmicroscopy.shoola.env.config.OMEROInfo;
import org.openmicroscopy.shoola.env.config.Registry;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something is not quite right in this class.
env.* classes should not have leaked in this more general package.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I introduced these imports when overhauling the login screen / server editor. Hm, gonna need quite some workarounds to get rid off them.

@dominikl
Copy link
Member Author

Just moved the login package from util to env as there are already other login / start up related ui classes like SplashScreen in env too.

@jburel
Copy link
Member

jburel commented Apr 2, 2020

Screenshot 2020-04-02 at 19 40 53

I am not convinced a user will under the message " no enum ..."
It will be better to have a message like protocol not supported: FTP

@dominikl
Copy link
Member Author

dominikl commented Apr 3, 2020

It looks like it wasn't build with my gateway PR ome/omero-gateway-java#34 then. With that PR it will then say "XYZ is not supported". Thought the merge build contains all open PRs of all repositories?

@jburel
Copy link
Member

jburel commented Apr 3, 2020

I will try again. I used yesterday's server build since I could not log in to merge-ci

@jburel
Copy link
Member

jburel commented Apr 3, 2020

The dialog pops up twice with the same message if you click ok the first time
then click login again

@jburel
Copy link
Member

jburel commented Apr 3, 2020

Screenshot 2020-04-03 at 08 39 50

I was using the branch directly while trying to debug a connection issue. The message is better now

@mtbc
Copy link
Member

mtbc commented Apr 3, 2020

error

Works well for me too but could have been more timely when I entered the server name rather than after I'd added it to the list and actually tried to connect with my username and password. Still an improvement though!

@dominikl
Copy link
Member Author

dominikl commented Apr 3, 2020

Moving of the login package to util has probably caused that one looses all the previous server settings because the java prefs location is determinded by the classname. I'll add another commit to fix that.

@jburel
Copy link
Member

jburel commented Apr 3, 2020

@mtbc when the server name is entered, there is no interaction with the gateway at that stage
Changing that will add complexity not really needed at this stage

@dominikl
Copy link
Member Author

dominikl commented Apr 3, 2020

The problem with the previous server list being lost is indeed due to moving the ServerEditor class into a different package. Shall I just close the PR? Not worth spending lots of time on this.

@jburel
Copy link
Member

jburel commented Apr 7, 2020

Let's fix the protocol error notification problem in this PR
We can look at re-organising the package in another PR.
I should have noticed that before

@dominikl
Copy link
Member Author

dominikl commented Apr 7, 2020

Added a simple workaround to load the old server list. This can simply be removed again in a later release.

Preferences prefs = Preferences.userNodeForPackage(ServerEditor.class);
List<String> servers = getServersFromPrefs(prefs);

// Workaround to load old preferences (remove again in next release):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's only true if the user definitely installs this version, correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true... maybe 'next release' is a bit too optimistic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit pity that one needs the class to really exist to load the Preferences, afaik not possible to just load by name.

@jburel
Copy link
Member

jburel commented Apr 8, 2020

Screenshot 2020-04-08 at 11 49 08

The list is back

@jburel
Copy link
Member

jburel commented Apr 9, 2020

I will include that PR and prepare a release of gateway next week. Then bump the version

@jburel
Copy link
Member

jburel commented Apr 14, 2020

Small problem I noticed

The latest server is not remember e.g.

  • I connect to workshop
  • disconnect
  • start insight again
  • localhost is displayed

@dominikl
Copy link
Member Author

Sorry, totally missed the last comment. I'll have a look at the issue today.

@dominikl
Copy link
Member Author

dominikl commented Jun 3, 2020

I think the issue #113 (comment) was introduced with #66 . Trying to find out how, then I can fix it in this PR.

@jburel
Copy link
Member

jburel commented Jun 23, 2020

@dominikl do you want to fix in this PR?
I think I will do a release as soon as #137 is in

@dominikl
Copy link
Member Author

#113 (comment) should be hopefully fixed now by the last commit.

@jburel
Copy link
Member

jburel commented Jun 29, 2020

The latest server is selected. The last commit works.
I still have cases when the dialog pops again after clicking "ok"
The first time it works fine, the second time it shows up twice.

@dominikl
Copy link
Member Author

There was some weird code which had the effect the ActionListener was added multiple times. So this should be fixed now too.

@jburel
Copy link
Member

jburel commented Jul 1, 2020

Looks good now

@jburel jburel merged commit 48dad36 into ome:master Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants