magmaOffenburg / RoboViz

Monitor and visualization tool for the RoboCup 3D Soccer Simulation League
Apache License 2.0
50 stars 17 forks source link

Added the possibility to add new connections (incl. port) #105

Closed strouble closed 5 years ago

strouble commented 5 years ago

We sometimes start multiple games on a server (different ports) an want to monitor them in one roboviz instance.

Gama11 commented 5 years ago

The diff is a bit hard to read because it uses different indentation / deviates from the existing formatting.

We use clang-format for code formatting (there's a config file in the root directory). It can be integrated into your IDE (setup instructions here) or simply be run as a command-line tool.

strouble commented 5 years ago

should be fixed

Gama11 commented 5 years ago

Something doesn't seem quite right with the addresses added by the "Connect to" dialog (127.0.0.1 in this case), they're rendered differently than the ones from my config.txt:

Also, wouldn't it make more sense for new connections to be added on the bottom?

Gama11 commented 5 years ago

I wonder how useful this feature is in practice, since there's no option to persist connections to the config (admittedly it wouldn't map well to how how the connection settings are currently structured since port and host are separate).

Don't you end up having to re-enter the same hosts/ports a lot when restarting RoboViz? Or are those typically different each time anyway for your use case?

strouble commented 5 years ago

Most of the time i just run simspark on my local machine, but i had recently the problem to quickly switch between different instances. Multiple simspark instances were generated automatically (with different ports on the same server) and i found no option to change the connection without restarting RoboViz. And yes, i had to re-enter the host/port, but each new game got a new port and i had to switch anyway. Persisting those connection wouldn't be usefull too, because the connection would only be used again at random (in my case).

Gama11 commented 5 years ago

I see, for randomly selected ports that indeed makes sense. 👍

Seil0 commented 5 years ago

I wonder how useful this feature is in practice, since there's no option to persist connections to the config (admittedly it wouldn't map well to how how the connection settings are currently structured since port and host are separate).

maybe someone could add the possibility, to save host and ports, later?

strouble commented 5 years ago

I've fixed most of the remarks.

The reason for the different rendering is how the ui is initialized:

The solution to that could be to set the L&F earlier, or to "hack" it somehow in order to switch back if needed.

maybe someone could add the possibility, to save host and ports, later?

That was my thought, too. For example, a checkbox can be added to the connection dialog to enable persisting the new connection. This gives the user full control. Therefore the config have to change to save host & port and that is a new feature in my opinon.

Gama11 commented 5 years ago

Oh, interesting, so it's actually the existing menu items that are rendered incorrectly. Seting the L&F as early as possible sounds sensible.

The idea of a checkbox came to my mind as well, but I agree that shouldn't be part of this PR.

strouble commented 5 years ago

is there something else todo? Otherwise, I see it finished on my part.

Gama11 commented 5 years ago

It sounded like you still want to fix the Look&Feel?

strouble commented 5 years ago

ok, pushed my changes for the L&F

Gama11 commented 5 years ago

Thanks!