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

Improve the city dialog unit lists #1201

Merged
merged 4 commits into from
Aug 7, 2022

Conversation

lmoureaux
Copy link
Contributor

Closes #805
Closes #1200

Test plan

  • Try to repro the above issues

The icon_list widget is only used to display a list of units. Rename it as
such. Also implement the context menu and filling the list directly in the
class instead of externally, saving a few lines of code and fixing longturn#1200.

Closes longturn#1200.
It was previously operating on single items. In order to allow for multiple
selection, we need to handle this at the list level instead.

See longturn#805.
It allows for much faster operation than dealing with each unit individually
(for instance, upgrading all units of a certain type). Multiple selection uses
the OS' standard shortcuts and also accepts dragging the mouse over multiple
units to select them all.

Closes longturn#805.
This class had become so small that it was no longer worth it.
@lmoureaux lmoureaux requested a review from jwrober August 6, 2022 17:54
Copy link
Collaborator

@jwrober jwrober left a comment

Choose a reason for hiding this comment

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

1200 is completely fixed, however 805 is only partially done. The issue asks for an activate unit and activate unit and close option similar to what fc gtk client supports. Right now we get activate, which when selected closes the city dialog. I am ok with approving this PR, but we should open a subsequent issue enhancement asking for a change to the selection menu.

@lmoureaux
Copy link
Contributor Author

805 is only partially done

I thought we had agreed in #805 that multiple selection also solved the use case?

@jwrober
Copy link
Collaborator

jwrober commented Aug 7, 2022

805 is only partially done

I thought we had agreed in #805 that multiple selection also solved the use case?

Oh I missed that. I didn't scroll down. I am good then. I will need to update the manual, this is not totally obvious

@jwrober jwrober merged commit f9865c5 into longturn:master Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants