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

Use shift + click to show vehicles' group. #7582

Closed
wants to merge 5 commits into from

Conversation

stormcone
Copy link
Contributor

AFAIK currently if we would like to do something with the vehicles in a vehicle's group, we have to find the vehicle in the vehicle group window to see the group's name it belongs to. Than we have to find that group in the list and select it, and just after that can we use the manage list functions. I think this is a little bit circumstantial.

This PR tries to make that process easier. We can use shift + click in the vehicle group and in the vehicle view window to select the vehicle's group.

@nielsmh nielsmh changed the title Use shitf + click to show vehicles' group. Use shift + click to show vehicles' group. May 11, 2019
@stormcone
Copy link
Contributor Author

I fixed a bug when the vehicle's group were a subgroup and it's branch were collapsed.

And I added one more commit with a similar functionality.

@nielsmh
Copy link
Contributor

nielsmh commented Jun 9, 2019

I don't like "Right click on a news items opens group list". The right mouse button has traditionally (in TTO and TTD) been reserved for panning the map, and showing tooltips/help boxes. This would go against that convention.

@stormcone
Copy link
Contributor Author

@nielsmh I prefer to play this game with mouse, I use the keyboard rarely. And I wanted to make this function to be easy to access, that is why I chosen the right click. But if most of you do not like it, maybe we can try out double click instead. (Or using a separate button, putting on the left side of the title bar or somewhere else.)

@stormcone
Copy link
Contributor Author

I fixed a problem when assert was triggered because there were no groups.

I added another commit which adds a button to the vehicle advisory news window to open the group window. This is an alternative solution to the right-click commit. So if you better like this then I can delete the other one.

@andythenorth
Copy link
Contributor

Thanks for this. Whilst I see the case for this, I think it presents a few UI problems, and there has also been no activity on the PR for a few months.

We try to keep the PR count low to keep the project healthy, so I'm closing this one. Thanks for contributing!

@stormcone
Copy link
Contributor Author

May I ask what kind of UI problems do you have with it? I think the first 3 commit do nothing with the UI. The 4th maybe have, but can be chosen one of the 4th and 5th to include it.

And there was no activity from my part because I think it's finished and I waited for another review.

@andythenorth
Copy link
Contributor

Ok, let's re-open this :)

I can't judge code quality, but I had a few comments.

Key choice (UI)

  • it would make more sense to use ctrl as the modifier key, as that's established for things like 'open related item x' (e.g. ctrl-click station in orders moves map to station)
  • shift is primarily for showing construction costs and should remain that way (there are probably exceptions, but I don't think this is one)
  • however, ctrl-drag is already used in vehicle list: it adds to the group all vehicles sharing orders
  • I would have no problem with overloading ctrl so that ctrl-drag and ctrl-click both perform different actions in the vehicle list, but I don't know if others will agree

Extending the behaviour (out of scope for this PR probably)

  • there are vehicle lists for vehicles using a station, and vehicles with shared orders
  • these list the group for each vehicle, but there's no easy way to get to the group (nor is there a way to open an auto-replace window, which has to be done instead on the group window)
  • extending this new behaviour to open groups from the station vehicle list and shared orders vehicle list would be really helpful, but I don't know if it's possible
  • if it is possible to extend the behaviour, it's probably better as another PR?

Thanks!

@stormcone
Copy link
Contributor Author

Ok, let's re-open this :)

So could you re-open this one or should I open a new PR?

* however, ctrl-drag is already used in vehicle list: it adds to the group all vehicles sharing orders

Yes, that the reason I originally chose 'shift' instead of 'control'. But now I changed it to 'control'.

* extending this new behaviour to open groups from the station vehicle list and shared orders vehicle list would be **really** helpful, but I don't know if it's possible

It can be done easily.

@andythenorth
Copy link
Contributor

I can't re-open, don't know why. I'll ask.

@LordAro
Copy link
Member

LordAro commented Oct 25, 2019

GH refuses to reopen, I'm afraid - "The shitf-click-group branch was force-pushed or recreated.". You'll need to create a new PR

@stormcone
Copy link
Contributor Author

Then I open a new PR.

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

4 participants