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

Move card db update menu entry #4015

Merged
merged 1 commit into from
Jun 29, 2020
Merged

Move card db update menu entry #4015

merged 1 commit into from
Jun 29, 2020

Conversation

tooomm
Copy link
Member

@tooomm tooomm commented May 26, 2020

Short roundup of the initial problem

Update Client and Update Cards are two separate processes.
Users often don't find them, or are not aware that there are two as they only realize one in the menus.

What will change with this Pull Request?

  • Put them next to each other in the Help menu
  • I reordered some lines to make them match the appearance in client

Screenshots

helpmenu

@tooomm
Copy link
Member Author

tooomm commented May 26, 2020

The Check for Card Updates... option to run Oracle could also be placed in the Card Database menu.
People are more likely to expect and find it there and might look there more regular than the Cockatrice main menu.

Both are better places than the current one in my opinion.
The Cockatrice menu is barely used after registering once and setting up auto-connect.

@ebbit1q
Copy link
Member

ebbit1q commented Jun 1, 2020

You're making some assumptions there, I'd prefer this placed in its own menu called Updates with an option to open up release notes. That way noone wonders which menu to use for updating.

@tooomm
Copy link
Member Author

tooomm commented Jun 1, 2020

I'd prefer this placed in its own menu called Updates...

Hmm, I understand where you are going. But generally such update actions are often placed in the Help or About menu for apps with a normal menu (e.g. Adobe applications). I can only tell for Windows.

Furthermore, the menu would only keep two options for now. As soon as we get automatic update checks, all kind of update processes would probably be combined to one action which lists all available updates. Also with additional information hopefully.
Do you want to keep the menu then with only one option in it?

...with an option to open up release notes.

Users can open the changelog/release notes before updating already:
rnotes


I think this is an improvement over the split up entries, without rethinking all menu.
With the new update routine and automated checks we can see what makes most sense then?

@tooomm
Copy link
Member Author

tooomm commented Jun 25, 2020

How do we proceed here? @ZeldaZach

There is no reason a user has to search in different places in order to use the most recent client and data.

I see two potential options for improvements for now:

  • Group both in the Cockatrice menu (aka move client updates there)
  • Group both in the Help menu as other apps have it there, too (aka move card updates there) That's what this PR proposes

Ebbit suggested a third, which I'm not a fan of as outlined above.

@ZeldaZach
Copy link
Member

My suggestion would be to move them all to the Help menu, which this PR solves. As such, I'm fine going ahead with this change.

@ebbit1q
Copy link
Member

ebbit1q commented Jun 26, 2020

I see it does some reordering as well, do these not affect the order in which the menus are displayed?

@tooomm
Copy link
Member Author

tooomm commented Jun 26, 2020

I did the reorder exactly for that reason - appearance in code matches now the appearance in client. :)
Displaying order is defined in a different spot, so one could argue that the actions should be alphabetical. It did not match either pattern and looked random, so I decided to go with the "order them the way they appear in client".

Do you want me to change that? @ebbit1q
Is it better to have them in e.g. alphabetical order when working with the code and adding new stuff for example?

@ebbit1q
Copy link
Member

ebbit1q commented Jun 29, 2020

Order they appear in client, they look random because they were most likely added over time.

@ebbit1q ebbit1q merged commit 2a7268c into master Jun 29, 2020
@ebbit1q ebbit1q deleted the tooomm-patch-7 branch June 29, 2020 08:25
@tooomm
Copy link
Member Author

tooomm commented Jun 29, 2020

As a positive side effect, updating cards/tokens is now uniform on all platforms:
https://github.com/Cockatrice/Cockatrice/wiki/Frequently-Asked-Questions-(FAQs)#how-do-i-update-my-card-database-to-have-the-latest-cards

longagofaraway pushed a commit to longagofaraway/Cockatrice that referenced this pull request Aug 14, 2020
* move db udpate (Cockatrice#4015)

* Remove gitlab config (Cockatrice#4037)

* revert Cockatrice#2345

* remove gitlab yml

* fix message when moving cards to bottom of library (Cockatrice#4006)

* Change method of opening directories to be the same for all oses, including linux (Cockatrice#4046)

* add opening directory in file browser to linux

this uses QDesktopServices to open the url "file://[location]"
by default this is
"file://$HOME/.local/share/Cockatrice/Cockatrice/pics/CUSTOM"

any distro that has a file browser should have an accompanying mime type
specifying the file handler for the file:// protocol using the
inode/directory mime type

see https://specifications.freedesktop.org/shared-mime-info-spec/shared-mime-info-spec-latest.html

if a user were to have removed their mime database this will not work and
it will fail with nothing but a log message, this would be rare and not
worth checking in my opinion

* make opening directories the same for all oses

* sort headers

* Made user information window resizable (Cockatrice#4009)

* Added horizontal layout and stretch for player icon (Cockatrice#4052)

* Fix unresolved symbols when link tests to system libgtest-dev (Cockatrice#4055)

* Enable parallel compilation. (Cockatrice#4057)

* travis: update macos 10.15 images (Cockatrice#4059)

* Fix release tests (Cockatrice#4063)

Co-authored-by: tooomm <tooomm@users.noreply.github.com>
Co-authored-by: Lee Tran <54418451+LeeTranMN@users.noreply.github.com>
Co-authored-by: ebbit1q <ebbit1q@gmail.com>
Co-authored-by: awlangham <awlangham@gmail.com>
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.

None yet

3 participants