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

PDS refresh improved controls with GHK feature #12898

Merged

Conversation

riverwanderer
Copy link
Collaborator

@riverwanderer riverwanderer commented Nov 11, 2023

Expanded "orphan counter" functionality - reporting / ability to repair (sub-option).
Ability to do additional processing via a special post-refresh Global Hotkey, optionally without routine refresh reporting.
New filter field and associated functionality for PDS Refresh.
Other UI improvements.

New options & sub-options shown. Actual defaults are unchanged from vassal v3.7.5:
image

Addresses an outstanding part of #11999.

Other PDS Refresh improvements:
Control panel re-organised and window title upgraded to a Progress bar.
Removed faulty "test" option & non-functioning "delete when no map" option.
Alert option for end of refresh run (using default Vassal system wake-up sound).
Avoids duplicate file entries.
Error / warnings summary collated and reported at end of run.
Sets the GameModule dirty flag if a refresh is done.

Improvements to standard Refresh Counters:
Each of the warning messages is only issued (now highlighted in red) if 1 or instances of the error/anomaly are detected.
Control panel updated for consistency with PDS Refresh.

riverwanderer and others added 30 commits September 14, 2023 19:40
…fresh is opt-in, PDS referesh is opt-out. Deck refresh options are formatted to distinguish them from main options.
…stPreControls2

# Conflicts:
#	vassal-app/src/main/java/VASSAL/build/module/GameRefresher.java
#	vassal-doc/src/main/readme-referencemanual/ReferenceManual/GameRefresher.adoc
…verwanderer/vassal into 12260-refreshPostPreControls2

# Conflicts:
#	vassal-doc/src/main/readme-referencemanual/ReferenceManual/GameRefresher.adoc
… Rationalise option settings. Attempt to close (dispose) options window at the end of processing.
…rmat (deck) sub-options. Make delete no map pieces default (consistent with Refresh Counters). Rationalise option settings & use constants always. Doc spelling & grammar tweaks.
…ssal into 12260-refreshPostPreControls2

# Conflicts:
#	vassal-app/src/main/java/VASSAL/build/module/GameRefresher.java
#	vassal-doc/src/main/readme-referencemanual/ReferenceManual/GameRefresher.adoc
…f PDS to be processed). For module development.
@riverwanderer riverwanderer added Ready for Review Ready to be reviewed for Merging and removed On Hold Waiting for other changes labels Dec 6, 2023
…x to GpidChecker.noGpIdMatch is appropriate and that the related declarations in GameRefresher are also ok (lines 118-124).
@claudiociardelli
Copy link
Collaborator

Hello Mark,
I am happy to see that you have been working on the PDS refresh tool. I have worked on this topic 2 or 3 yrs ago and am still receiving some github notifications.
Hope you are getting on top of this complicated feature. I do not remember the details any more, but I remember that it was complex.
Kr
Claudio

@riverwanderer riverwanderer removed the request for review from claudiociardelli December 7, 2023 13:03
@uckelman uckelman modified the milestones: 3.7.6, 3.7.7 Dec 8, 2023
@riverwanderer riverwanderer requested a review from uckelman January 8, 2024 18:02
@riverwanderer
Copy link
Collaborator Author

riverwanderer commented Jan 10, 2024

I am happy to see that you have been working on the PDS refresh tool. I have worked on this topic 2 or 3 yrs ago and am still receiving some github notifications.

Thanks @claudiociardelli - I have had great benefit from that work over those years! More recently, since I've started to dip my toe into Java and Vassal itself, through this PR I've added mainly peripheral features that I thought would be useful to make PDS-related workflow even more efficient.

In the core refresh, apart from an optional, non-default ability to "repair" orphan pieces (no matching GPiD), I've only tweaked the reporting output. I'd like to add ability to populate decks and at-start stacks but time to explore that eludes me at the moment.

@uckelman uckelman modified the milestones: 3.7.7, 3.7.8 Jan 11, 2024
int noMapCount; // shared to PDS refresher - not used!!!
int notOwnedCount; // shared to PDS refresher
int notVisibleCount; // shared to PDS refresher
int deckWarnings; // shared to PDS refresher
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the deal with these? They need a visibility modifier. Should they be private? protected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These variables and the 3 immediately preceding them are various types of error accumulated during a refresh run. Visibility must at least allow PreDefinedSetup.java to access them - it uses them to return an error count to RefreshPreDefinedSetupDialog. This is all so that the user gets a summary at the end rather than having to trawl through all the refresh logs.

Private doesn't work so I guess Protected ? IntelliJ doesn't approve of that and warns "Class member declared 'protected' in 'final' class" hence I accepted it's suggestion, which removed the Protected modifier:

image

i can added the Protected modifiers back in unless there's some other way I should be doing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

All see that you're doing with these variables outside of GameRefresher is summing them in one location. If that's all you need, then they should be made private and you can create a public method inside of which you compute the sum, and then call that method from outside of GameRefresher.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I've pushed the fix to this.

Whilst doing the fix, I've noticed that somehow in a commit on 7th Dec, I've managed to over-write the change history in GameRefresher. I presume it's happened whilst I was working around a glitch in IntelliJ. I think I may need to unwind or re-do this PR to fix this.

image

@uckelman uckelman merged commit e84358c into vassalengine:master Jan 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request Ready for Review Ready to be reviewed for Merging
Projects
None yet
3 participants