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

UnitReadoutDialog based on AbstractDlg #2910

Merged
merged 5 commits into from
Jun 10, 2021

Conversation

SJuliez
Copy link
Member

@SJuliez SJuliez commented Jun 4, 2021

As the title says. No functional change. Separates out the readout dialog from the lobby's "View" menu option into its own class based on AbstractDialog.
I'll say this was effortless.

@SJuliez SJuliez requested a review from Windchild292 June 4, 2021 21:16
@SJuliez SJuliez marked this pull request as draft June 4, 2021 21:20
@SJuliez
Copy link
Member Author

SJuliez commented Jun 4, 2021

Oh wait - forgot to rename the dialog with its own resource entries!! Grrr.

@Windchild292
Copy link
Contributor

Taking a look... other than the above, is there any reason to not just have this dialog wrap around entity view pane? Seems to share the same information but would add the TRO component too.

@SJuliez
Copy link
Member Author

SJuliez commented Jun 4, 2021

Thats the one used in the unit selector, right? I'll look into that. I was just continuing with what the lobby historically did, but good idea that. I wouldnt even bet against there already being such a dialog somewhere...

@Windchild292
Copy link
Contributor

Windchild292 commented Jun 4, 2021

It's the one I made that I forgot to replace the unit selector one with...
Made it for MekHQ's unit market based on (as in... nearly a direct copy of) the older unit selector.

And I'm almost certain the standardized dialog doesn't exist

@SJuliez
Copy link
Member Author

SJuliez commented Jun 5, 2021

OK, update:

  • Based the EntityReadoutDialog on the EntityViewPane
  • Used EntityViewPane in the AbstractUnitSelector
  • Modernized the MechViewPanel (and gave it a bit of space to the scrollbar, made it scroll even when the mouse wheel is used in the dialog free space or on the fluff image)

It's a bit hacky to set the title of an AbstractDialog to something variable but I don't mind that line.

I would have liked to make it scale but since the whole Unit Selector currently doesn't, this would look strange there.

@SJuliez SJuliez marked this pull request as ready for review June 5, 2021 07:45
@SJuliez
Copy link
Member Author

SJuliez commented Jun 5, 2021

Ah I forgot: Although the EntityViewPane->AbstractTabbedPane seems to want to save the index of the selected pane, this doesn't seem to work, neither for the unit selector nor for the separate dialog. I don't know why.

Copy link
Contributor

@Windchild292 Windchild292 left a comment

Choose a reason for hiding this comment

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

In general this looks great, just one oddity and a missing override

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.

2 participants