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

updated battle list sort chains and color highlights #913

Closed
wants to merge 7 commits into from

Conversation

@springraaar
Copy link

commented Feb 16, 2019

  • when sorting by "game", sort by : game prefix (without version), players DESC, game
    (this will highlight the most populated rooms for each game)

  • when sorting by "players", sort by : players, game ASC
    (this will show the most populated rooms first, but then show the remaining rooms sorted by game)

  • show grey text on room rows without players to make the ones that do have players stand out (default text color is black)

  • on windows (as setBackgroundColour doesn't do anything on SL on ubuntu), change the default row background from white to a shade of light grey that depends on the game prefix, to differentiate between sets of rows for different games

@springraaar springraaar reopened this Feb 17, 2019
@springraaar springraaar reopened this Feb 17, 2019
@springraaar springraaar force-pushed the springraaar:master branch from 82b52cc to bb06ea3 Feb 17, 2019
@springraaar

This comment has been minimized.

Copy link
Author

commented Feb 17, 2019

minor cleanup and some changes:

  • sorting by players sorts by active players first and then by total players (including spectators)
  • differences for color background for different games should be a bit sharper
@silentwings

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

Sorting in "hidden" ways that the user does not expect seems like a bad idea to me & imho ignoring game version when sorting is bad.

A screenshot would be helpful, for the other changes.

src/gui/battlelist/battledataviewmodel.h Outdated Show resolved Hide resolved
src/ibattle.cpp Outdated Show resolved Hide resolved
src/ibattle.h Outdated Show resolved Hide resolved
@abma

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

sorry, i forgot, to submit my review, reviewed it already some time ago

@springraaar

This comment has been minimized.

Copy link
Author

commented Feb 23, 2019

Sorting in "hidden" ways that the user does not expect seems like a bad idea to me & imho ignoring game version when sorting is bad.

A screenshot would be helpful, for the other changes.

The added comparisons only run as "tie-breaker". The game version is the specific case where, when sorting by game first, it's applied as a tie breaker after the game prefix and player count comparisons, to get a sorted set of rows like this

game | players
...
ba 10.x | 10
ba 9.46 | 9
ba 10.x | 5
ba 9.46 | 3
ba 10.x | 1
ba 10.x | 0
ba
...

instead of this

game | players
...
ba 10.x | 10
ba 10.x | 5
ba 10.x | 1
ba 10.x | 0
ba 9.46 | 9
ba 9.46 | 3
ba
...

The first is much easier to read. Also it would be confusing to have to use different colors to highlight the rows for different versions of the same game.

Here's some screenshots on ubuntu (unfortunately the row background colors don't work there)

  • sort by players
    sl_ubuntu_battlelist_players_desc

  • sort by game
    sl_ubuntu_battlelist_game_asc

@springraaar

This comment has been minimized.

Copy link
Author

commented Feb 23, 2019

the colors produced by the formula range from white to whitish grey, grey-blue and grey-yellow (r=g avoids pink :)). And they seem significantly different for the different games that end up adjacent on the battle list in most cases (I tested by setting the text color), but maybe I should use a simple sum % 10 as index to pick a color from an array of colors.

@silentwings

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

The first is much easier to read.

No, that's just personal perference. I dislike the first and like the second. Regardless, what matters is:

If I press a button telling the lobby to sort by "game", I want the list sorted by the visible contents of the "game" column and not anything hidden to do with player counts, and absolutely not anything which actually ignores part of my instruction.

Doing stuff that isn't what the user asked for is a bug.

@springraaar

This comment has been minimized.

Copy link
Author

commented Feb 24, 2019

The first is much easier to read.

No. I thoroughly dislike the first and like the second. Also, what matters is:

If I press a button telling the lobby to sort by "game", I want the list sorted by the visible contents of the "game" column and not anything hidden to do with player counts, and absolutely not anything which actually ignores part of my instruction.

Doing stuff that isn't what the user asked for is a bug.

In many UI cases the useful ways to sort tables involve a chain of comparisons to break ties, not a single comparison.

The results are still very close to "what the user asked", the exception only applies to sorts within the same game. Basically I want that when people sort by game the most populated rooms for each game are the first ones shown, regardless of whether people are playing one of various hosted versions. Different versions of the same game ARE the same game. The infighting about BA versions should be irrelevant to anyone not deeply involved in BA.

We're probably not going to agree. So, abma? what should I do?
1 - sort by game (inc. version) first, then players (what silentwings is asking EDIT: or not)
2 - sort by game prefix (exc. version) first, then players, then game (inc. version) (what I did)
3 - use (1) as default but add the option to have (2)
4- use (2) as default but add the option to have (1)

Btw sometimes I see rows for empty rooms changing position every few seconds, this might be because of a lack of "tie breaker" comparison, maybe by host name or something.

@silentwings

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

1 - sort by game (inc. version) first, then players (what silentwings is asking)

I hope its clear that this isn't what I said in #913 (comment). Any unnecessary sorting against/beyond what the user actually requested is a bug, including hidden tiebreak criteria. (Currently you can choose a tiebreak by pressing column headings one after the other.)

@springraaar

This comment has been minimized.

Copy link
Author

commented Feb 24, 2019

1 - sort by game (inc. version) first, then players (what silentwings is asking)

I hope its clear that this isn't what I said in #913 (comment). Any sorting against/beyond what the user actually requested is a bug, including hidden tiebreak criteria. (Currently you can choose a tiebreak by pressing column headings one after the other.)

pressing headings one after the other seems to have some effect, but seems unreliable. Springlobby only highlights the last one, so that behavior is hidden anyway (I've been using SL for many years and didn't even know it did that). To me the explicitly adding the tie breakers is a desirable feature, not a bug.

What sort rules do you normally use?

@silentwings

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

It varies depending on what I want to see...

pressing headings one after the other seems to have some effect, but seems unreliable

Yes, seems it doesn't always work.

@springraaar

This comment has been minimized.

Copy link
Author

commented Feb 24, 2019

what do you use most of the time?

@abma

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

you know about 7abd4fa?

this change merged game version and game name which makes the problem you trying to fix here apear...

-> why not split game name and version?

@springraaar

This comment has been minimized.

Copy link
Author

commented Feb 26, 2019

you know about 7abd4fa?

this change merged game version and game name which makes the problem you trying to fix here apear...

-> why not split game name and version?

Thinksome asked me that too. I've only recently begun tampering with SL code and aim to do simple stuff that affects player experience with minimal changes to code if possible to avoid adding bugs. I think refactoring is beyond the scope of this.

@springraaar springraaar force-pushed the springraaar:master branch from f142477 to 8f844ae Feb 27, 2019
@springraaar springraaar force-pushed the springraaar:master branch from 98aaa47 to 4ec0eb9 Feb 27, 2019
@springraaar

This comment has been minimized.

Copy link
Author

commented Mar 3, 2019

so...?

@abma

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

you've asked why this isn't merged:

its not merged because it possible introduces new problems but doesn't solve a problem or at least, i don't see which problem it solves.

@springraaar

This comment has been minimized.

Copy link
Author

commented Mar 6, 2019

Problems it solves:

  • makes battle list much easier to read at a glance (this is an enhancement, not a bugfix)
  • the comparison tie-breaker fixes the bug where rows shift position every few seconds

Problems it potentially creates:

  • some devs may not like the background color that gets assigned to their game, but that can be easily tweaked
  • people who customized SL colors may have a slight amount of trouble reading the grayed out text on inactive rooms (the grey is closer to black than to white, so this shouldn't be an issue unless they fully inverted the color scheme, if that's even possible..)
@silentwings

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

the comparison tie-breaker fixes the bug where rows shift position every few seconds

This should be fixed with minimal arbitrary hidden sorting criteria - the four lines after

// tie-breaker based on unique battle host name

is sufficient.

Afaics that is the only part of this PR which doesn't introduce issues.

@springraaar

This comment has been minimized.

Copy link
Author

commented Mar 13, 2019

ping

@springraaar

This comment has been minimized.

Copy link
Author

commented May 29, 2019

i've resolved abma's requests. Please merge this.

@springraaar

This comment has been minimized.

Copy link
Author

commented Jul 13, 2019

ping

Copy link
Member

left a comment

please remove the submodule src/downloader/lib change from the pull request / merge current master branch into it.

Copy link
Member

left a comment

without the sorting changes + the requested changes i'll merge this

@@ -7,6 +7,8 @@
#include <climits>
#include <set>
#include "log.h"
#include "../utils/conversion.h"

This comment has been minimized.

Copy link
@abma

abma Sep 1, 2019

Member

why is this include needed? additional includes in header files should be avoided when possible.

#include "utils/slconfig.h"

SLCONFIG("/BattleListTab/InactiveRoomColor", "#888888", "Font color for battle rows with no active players");
SLCONFIG("/BattleListTab/UseSmartSorting", "1", "Apply secondary sorting rules when sorting by game or players");

This comment has been minimized.

Copy link
@abma

abma Sep 1, 2019

Member

afaik a bool type exists for SLCONFIG, no need for making this a string

sortingResult = battleA->GetHostGameNameWithoutVersion().compare(battleB->GetHostGameNameWithoutVersion());

// secondary sort by players
if (0 == sortingResult) {

This comment has been minimized.

Copy link
@abma

abma Sep 1, 2019

Member

IMHO 0 == ... is bad style. should be sortingResult == 0 instead.

@@ -754,6 +754,14 @@ void IBattle::SetHostGame(const std::string& gamename, const std::string& hash)
m_game_loaded = gamename.empty();
m_host_game.name = gamename;
m_host_game.hash = hash;

gameNameWithoutVersion = gamename.substr(0,gamename.find_last_of(" "));

This comment has been minimized.

Copy link
@abma

abma Sep 1, 2019

Member

this relies on games having no space in its version. this looks like a predetermined breaking point.

This comment has been minimized.

Copy link
@springraaar

springraaar Sep 2, 2019

Author

In those cases it will assume as prefix a larger string that contains part of the version.

at the moment that's only used to figure out the colors, so this won't really have any impact other than some weirdness when sorting and coloring the game (it may assume different versions as different games).

I don't remember any such case recently on the lobby server. Can you find any game on springfiles with spaces on the version?

What do you suggest?

@springraaar

This comment has been minimized.

Copy link
Author

commented Sep 2, 2019

Note: Thinksome already merged these changes into his.

@abma

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

ignoring the requested changes results in ignoring the pull request.

why this can't be merged:

  • sorting "randomly" is bad style
  • coding style (0==...)
  • additional includes in headers (includes should be in .cpp)
  • pr-downloader submodule was updated for no reason which leads to a merge conflict

thats maybe not all, as i don't see why i should further investigate when the above stuff is not fixed.

springlobby is bloated enough: a cleanup really is needed

@abma abma closed this Sep 7, 2019
@abma

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

also i don't see what problems the sorting changes fixes: IMHO it just introduces new problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.