Server List from JSON #3165

ZeldaZach · 2018-04-03T23:23:56Z

This PR will take the servers from the list on ~~the wiki~~ GitHub Public Servers JSON and import them to the client, thus removing the dependency on hardcoded values.

When the user "refreshes the servers", we will re-download the list and clear any server not found on the public list.

Nightfirecat · 2018-04-03T23:44:54Z

Not a fan of reading from wiki--anyone can create an account and vandalize that. Could we maintain a file in this repo (or a separate repo) to hold that information?

ancestral · 2018-04-04T06:14:13Z

You can do it through the wiki, as long as the page has admin-only access.

Other sites do this sort of thing. The Battle for Wesnoth does this for their download links when they release updates, and the system works well, especially for maintainers who are not well-versed with command line functions.

ancestral · 2018-04-04T06:16:31Z

Still, I think most pieces of software with update checks will utilize REST APIs (for example, GET http://cockatrice.us/api/servers might return simple JSON with a list of servers) and when things change, they will just edit the static file on the server.

tooomm

Can the refresh button behave the same as for manually updating spoilers in settings?

disable it during update process
change label to Updating... during that time

Otherwise there is no way a users knows that something is going on or finished processing.

Vafthrudnir · 2018-04-04T08:44:51Z

cockatrice/src/dlg_connect.cpp

+                    default:
+                        break;
+                }
+            }


I looping really necessary here? All it does is increment an index variable, which is used in a switch - wouldn't directly indexing the serverValues list be nicer?

short nameIndex = 1; short addressIndex = 5; short portIndex = 6; serverName = QString(serverValues.at(nameIndex)).remove(QRegExp("<a[^>]*>")).split("</a>").at(0); serverAddress = QString(serverValues.at(addressIndex)).remove("<strong>").split("</strong>").at(0); serverPort = QString(serverValues.at(portIndex)).remove("<strong>").split("</strong>").at(0); ...

Honestly, I wrote this on a whim and once it worked I put it up for review. Looking back, the loop is kind of pointless and I'm sure there's a better way

By the way I don't really like that we get this list from a displayed source; if someone would change the formatting of the text (e.g. make it italic) the code would also have to be changed. It would be much better imo if we either had a dedicated file for the server list or used an API as @ancestral proposed.

I'd be fine with having a dedicated file; where would we store it?

tooomm · 2018-04-04T08:52:53Z

Still, I think most pieces of software with update checks will utilize REST APIs (for example, GET http://cockatrice.us/api/servers might return simple JSON with a list of servers) and when things change, they will just edit the static file on the server.

That's not working for us I think.
As long as that API is not part of our https://cockatrice.github.io/ webpage hosted from gh pages.

You can do it through the wiki, as long as the page has admin-only access.

We would have to make all wiki admin-only. You can't do that for certain pages only I think?
Also that wouldn't help too much since all server owners would have to create an issue and somebody has to manually update the wiki.
If it's a file in a repo all owners can update for themselves and somebody just needs to push the merge button after checking. That's a simpler and better process and keeps the list a bit easier to maintain.

The wiki page was for pretty presentation purpose, its structure or content might change (like I added the up/down badges recently). The parsing code would need to reflect such changes. Or somebody could easily mess a change up without even noticing and break things for all users.
We are looking for a robust and simple data source here instead.
Others might not realize a bad change because no PR got created, nobody checked the change and no notifications about changes got send etc.

As spoken about in the initial issue, I prefer a github based file, too.
Placed in a separate repo, gh pages repo (in combination with API?), cockatrice app repo... within the Cockatrice org.

When the user "refreshes the servers", we will re-download the list from the wiki and clear any server not found on the public list.

We don't want to do that I guess? 😛
Players still want to connect to their private or custom servers (testing, friends, lgs, pro team...).

Custom created servers should always be kept. Do we need two lists here then?
A official, public one that we maintain and update. Plus a custom one the user can add to.
Like with cards etc.

Vafthrudnir · 2018-04-04T08:50:11Z

cockatrice/src/dlg_connect.cpp

+                    default:
+                        break;
+                }
+            }


By the way I don't really like that we get this list from a displayed source; if someone would change the formatting of the text (e.g. make it italic) the code would also have to be changed. It would be much better imo if we either had a dedicated file for the server list or used an API as @ancestral proposed.

tooomm · 2018-04-04T09:24:12Z

Also, the alphabetical ordering isn't preserved:

…e complexity

ZeldaZach · 2018-04-04T16:53:42Z

@tooomm Nothing I can do about the sorting issue; If we capitalize the server name on the page, it should work. We added manually and this is how QMaps work

Daenyth

The wiki is publicly editable. I think this is a bad idea. We should keep the data file in git and download from there.

ancestral · 2018-04-05T02:15:56Z

@Daenyth As I was trying to say, you would make the page private, editable only by admins or certain people.

Nightfirecat · 2018-04-05T03:18:00Z

@ancestral Far as I can tell, there is no "private, editable-only-by-admins" option for wiki pages. All pages are editable by any GitHub user who has access to the repo.

ancestral · 2018-04-05T03:20:39Z

@Nightfirecat It might depend on the wiki software you are running, but you can definitely add user groups in MediaWiki.

Nightfirecat · 2018-04-05T04:34:07Z

I'm referring to GitHub wiki, since that's where it's currently stored.

tooomm · 2018-04-05T11:01:05Z

You can limit edits to org members in the github wiki too, but then all server admins can't manage their data anymore on their own as outlined above since they are not involved in development mostly. As well as the other drawbacks which come with using the wiki as source. IMO it's not the best solution here.

Daenyth · 2018-04-05T11:47:02Z

I'd really prefer using git. It has a review process, it's public, and it's reliable. Even if we can lock the one wiki page to org members only.

tooomm · 2018-04-05T13:38:58Z

You can only lock all wiki as far as I know^^

+1 for git

ZeldaZach · 2018-04-06T02:34:29Z

Latest Commit: Will now use https://github.com/Cockatrice/cockatrice.github.io/blob/master/public-servers.json instead of the Wiki. This allows us to keep the server details in git, prevent issues with stylistic changes, and better determine the order shown in client.

ZeldaZach · 2018-04-07T16:41:13Z

@tooomm

Modifying a public server will not make it "custom". Only if you add via the "new host" method.
Yes, Dae told me to change it to use the URL instead of nick name
Because it's the easiest way for me to figure out what's going on for what to remove and what to keep. We only archive the name, so I'm fine with this.
Sounds good!

…lass includes instead of full includes, one NAM

…uccess/failure).

ZeldaZach · 2018-04-07T17:35:07Z

Ok, I've addressed everyone's comments... How's this looking now @Daenyth @ctrlaltca @tooomm ?

I've also added an option with this latest push to have a menu option that refreshes the servers. This will be the new default as of the 2.7.0 release (next next release) where we will remove the GUI button.

ZeldaZach · 2018-04-08T02:45:20Z

Any opinions on a one-time flush of the server list so it re-downloads the public? Or should we leave it as is? I just tested with an older .ini file and refreshed. While it added the new servers, the old ones were left behind (since they were "custom" and were old)... I'd like to flush them all first time through

tooomm · 2018-04-08T07:27:58Z

Yes, I realized that the old woods server didn't get deleted, too.
But without warning the user/giving a choice, I don't like flushing all their personal lists.
Or do you think the complains won't be too bad? We should have a prominent hint in release notes at least.

Another thing I run into, is that refreshing the list kills your selection and jumps to last host used. So if you swapped servers and entered new login details but decide to refresh servers first, all your new input data is lost and the selection jumps back to the last connected server.

tooomm · 2018-04-08T08:12:06Z

From Gitter:

Zach H @ZeldaZach Apr 07 18:28
A thought. Should I move the refresh server button to the menu instead of on the GUI

David Szabo @Vafthrudnir Apr 07 18:56
I think it's okay on the gui
Easily recognizable

Zach H @ZeldaZach Apr 07 18:57
I'm gonna try both for now
Because it's not a button people will press often

David Szabo @Vafthrudnir Apr 07 19:04
The main use-case will be IMO that when a user can't connect to their usual server,
They will click it and try again
And in this case it's better if the refresh button is right there instead of somewhere in a menu

Zach H @ZeldaZach Apr 07 19:07

I kind of find this better tbh

David Szabo @Vafthrudnir Apr 07 19:16
What happens when it's clicked? Does it just start the refresh process in the background?

Zach H @ZeldaZach Apr 07 19:30
Yes, and it will launch a popup on failure/success
I think, for this release, I'll include the button. Afterwards I'll remove it

I have to disagree here. I highly prefer the option within the related dialog instead a separate option in the main app menu.
What kind of improvement I like though, is to exchange the big text button for a small one right next to the drop down menu. Instead a big text label it can only be a "refresh/reload circle" icon similar to the one we use in the custom shortcuts page in settings. The big icon is a bit disruptive and distracting, and catches too much attention for a not so often needed feature:

A clear and helpful button hover label should provide a bit more additional info to the user like "Refresh the server list from a online source with provided public ones"

ZeldaZach · 2018-04-08T08:13:45Z

@tooomm I love that idea of re-using the "refresh" button from the shortcuts! I'll look into that today or tomorrow. If that's the case and we stick with it, I'll be removing the dialog option as it won't be needed.

ZeldaZach · 2018-04-09T03:48:55Z

Newest commit: Reverts the menu option, moves the button and uses the refresh icon next to the server list with a tool tip to let you know what it does. On successful run, it will ask the user if they want to clear their old list or not (Sorry @Daenyth :P).

Completed

Vafthrudnir

Otherwise seems good.

Vafthrudnir · 2018-04-09T12:38:26Z

cockatrice/src/window_main.cpp

@@ -1205,7 +1227,7 @@ int MainWindow::getNextCustomSetPrefix(QDir dataDir)

 void MainWindow::actManageSets()
 {
-    auto *w = new WndSets;
+    w = new WndSets;


I think a one-character variable name was okay in this local scope, but since now it's made into a data member of a quite large class it probably should get a more expressive name.

tooomm · 2018-04-09T14:17:42Z

Check #3182 please

ZeldaZach · 2018-04-09T20:36:58Z

cockatrice/src/handle_public_servers.cpp

+
+        bool serverFound = false;
+        for (const auto &iter : savedHostList) {
+            if (iter.first == serverName) {


TODO: Change this to match by URL

Daenyth

Appreciating the code cleanup with this 👍

Nice work!!

Daenyth · 2018-04-10T02:07:03Z

cockatrice/src/handle_public_servers.cpp

+        QString serverPort = serverMap["port"].toString();
+
+        bool serverFound = false;
+        for (const auto &iter : savedHostList) {


This would be clearer with std::find_if but this is also fine 👍

Never heard of it, so I'd rather leave it as-is for now. Will look into the standard functions though for future reference!

Daenyth · 2018-04-10T02:08:41Z

cockatrice/src/settingscache.cpp

    settings->setValue("chat/mention", chatMention);
 }

 void SettingsCache::setChatMentionCompleter(const int _enableMentionCompleter)
 {
-    chatMentionCompleter = _enableMentionCompleter;
+    chatMentionCompleter = (bool)_enableMentionCompleter;


Why not static_cast here?

Clion changed it to be like this. I'm assuming it has something to do with the const functionality and it might need a const_cast which is dangerous. C style probably avoids this. @ctrlaltca ?

The member var chatMentionCompleter is a boolean, while the _enableMentionCompleter parameter is an integer. Clion just made the "int to bool" cast explicit; no const involved.

Dynamic cast then? I thought our style was to not use the c-style paren casts any more in favor of the cpp casts

Daenyth · 2018-04-10T02:09:16Z

cockatrice/src/shortcutssettings.cpp

-        return false;
-    }
-    return true;
+    return !forbiddenKeys.contains(checkSequence);


Daenyth · 2018-04-10T02:12:13Z

I really appreciate everyone going through the multiple rounds of feedback; this is going to be a great feature for our users.

Nice work @ZeldaZach @Vafthrudnir @ctrlaltca @Nightfirecat @tooomm :D

Nightfirecat · 2018-04-10T04:01:29Z

Agreed, great work all, and especially great work @ZeldaZach for implementing!

tooomm · 2018-04-10T17:09:21Z

@Lachee @dev-id
FYI: the central file Cockatrice pulls its server data from is accessible trough this url, and stored/edited right in the github repo for our webpage:
https://github.com/Cockatrice/cockatrice.github.io/blob/master/public-servers.json

Please keep that updated in case of important changes (as well as the wiki page for public servers).

(@Lachee: Also, check your gitter pm's by chance 😉)

ZeldaZach added 6 commits April 3, 2018 18:11

use public server list on wiki to generate the dropdown list

0091066

temp

21b2347

resolve conflicts

3e2a06e

whoops

c4794cf

remove expired servers

197e5e1

disable on no select

f432ae2

ZeldaZach requested a review from Vafthrudnir April 3, 2018 23:23

ZeldaZach added 2 commits April 3, 2018 19:24

unneeded

9dda155

clangify

d099a30

tooomm previously requested changes Apr 4, 2018

View reviewed changes

ZeldaZach requested a review from Daenyth April 4, 2018 07:57

Vafthrudnir reviewed Apr 4, 2018

View reviewed changes

Vafthrudnir previously requested changes Apr 4, 2018

View reviewed changes

Fix the "Downloading..." issue with the refresh button and reduce tim…

994de96

…e complexity

Daenyth previously requested changes Apr 5, 2018

View reviewed changes

use json from cockatrice website repo instead of wiki

435e195

ZeldaZach added 2 commits April 7, 2018 12:47

Enable/Disable the button each time to prevent multiple clicks, use c…

ca8e86f

…lass includes instead of full includes, one NAM

Add dropdown menu option to Actions to refresh servers (w/ popup on s…

6d061b8

…uccess/failure).

ZeldaZach added 2 commits April 7, 2018 13:40

Clangify

4e4ea0c

small change to looks

b3ee904

change button, remove menu option, add option to clear old list

5350797

Vafthrudnir approved these changes Apr 9, 2018

View reviewed changes

tooomm mentioned this pull request Apr 9, 2018

crash if you try to connect without a pw entered #3182

Closed

ZeldaZach commented Apr 9, 2018

View reviewed changes

ZeldaZach added 4 commits April 9, 2018 17:07

User Server address instead of name when handling information

00bb9dd

clangify

5dd558b

remove custom

387cff6

Fix passwords being saved inadvertently, racetime fix

d81c2a6

Daenyth approved these changes Apr 10, 2018

View reviewed changes

ZeldaZach merged commit 61e5095 into Cockatrice:master Apr 10, 2018

ZeldaZach deleted the public_server_list branch April 10, 2018 02:38

tooomm mentioned this pull request Apr 10, 2018

!!! pw leak on every connect #3183

Closed

Server List from JSON #3165

Server List from JSON #3165

Conversation

ZeldaZach commented Apr 3, 2018 • edited Loading

Nightfirecat commented Apr 3, 2018

ancestral commented Apr 4, 2018

ancestral commented Apr 4, 2018 • edited Loading

tooomm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tooomm commented Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

tooomm commented Apr 4, 2018

ZeldaZach commented Apr 4, 2018

Daenyth left a comment

Choose a reason for hiding this comment

ancestral commented Apr 5, 2018 • edited Loading

Nightfirecat commented Apr 5, 2018

ancestral commented Apr 5, 2018

Nightfirecat commented Apr 5, 2018

tooomm commented Apr 5, 2018

Daenyth commented Apr 5, 2018

tooomm commented Apr 5, 2018

ZeldaZach commented Apr 6, 2018

ZeldaZach commented Apr 7, 2018

ZeldaZach commented Apr 7, 2018

ZeldaZach commented Apr 8, 2018

tooomm commented Apr 8, 2018 • edited Loading

tooomm commented Apr 8, 2018 • edited Loading

ZeldaZach commented Apr 8, 2018

ZeldaZach commented Apr 9, 2018

Vafthrudnir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tooomm commented Apr 9, 2018

Choose a reason for hiding this comment

Daenyth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daenyth commented Apr 10, 2018

Nightfirecat commented Apr 10, 2018

tooomm commented Apr 10, 2018

ZeldaZach commented Apr 3, 2018 •

edited

Loading

ancestral commented Apr 4, 2018 •

edited

Loading

tooomm commented Apr 4, 2018 •

edited

Loading

ancestral commented Apr 5, 2018 •

edited

Loading

tooomm commented Apr 8, 2018 •

edited

Loading

tooomm commented Apr 8, 2018 •

edited

Loading