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

[Maintenance]: Remove campaign export functionality #4047

Closed
kwvanderlinde opened this issue May 9, 2023 · 8 comments
Closed

[Maintenance]: Remove campaign export functionality #4047

kwvanderlinde opened this issue May 9, 2023 · 8 comments
Assignees
Labels
code-maintenance Adding/editing javadocs, unit tests, formatting.

Comments

@kwvanderlinde
Copy link
Collaborator

Describe the problem

The campaign export has an awkward existence in today's MapTool.

  1. The set of supported versions seems arbitrary to a user (it only allows 1.4.0 through 1.5.1). There's no support for exporting to more recent versions that users are more likely to encounter today.
  2. The supported versions predate the 1.8.3 security fix. We've sternly warned users not to use such versions of MapTool, so it seems irresponsible to offer this invitation to downgrade.
  3. The functionality has not been maintained and no longer functions properly. The exporter essentially operates by modifying the data to remove things the older versions don't expect. This has not been kept up over time, so there is now lots of stuff that has been added which is not removed by the exporter. I cannot today take my 1.13.0 campaign, export it to 1.4.0.0 and expect it to work.
  4. The campaign exporter is a maintenance burden. While not a large amount of code, it pops up when doing searches and the like, and just serves to make developers second-guess whether they're supposed to do anything with it.

The improvement you'd like to see

The campaign export is removed in its entirety.

Expected Benefits

One less thing to have to think about and wade through.

Additional Context

I'm also noticing ModelVersionManager is in a similarly awkward position, though it's invisible to the user and only handles upgrades. Still, it only has logic for really old versions (1.3.51 - 1.3.78), is more invasive than the campaign export logic, and I believe the handful of XML transforms that it does can instead be handled via readResolve().

@thelsing
Copy link
Collaborator

thelsing commented May 9, 2023

+1 for both points

@kwvanderlinde kwvanderlinde added the code-maintenance Adding/editing javadocs, unit tests, formatting. label May 9, 2023
@Azhrei
Copy link
Member

Azhrei commented May 9, 2023

I had a plan very early on to host "plugins" on our web site that would be downloaded on first use of a new version and cached. Those plugins would basically be pieces of code that could convert the "current" campaign file format into one for the "previous" format. Chaining these things together could allow any newer version to import/export any older version. But I never implemented that plan. Given how out of date the code is now, and how terrible the design is (loading the entire XML string into memory to make changes?!), I also agree with just removing it.

@kwvanderlinde
Copy link
Collaborator Author

Okay, the campaign export is really easy tear out ...

Moving on to ModelVersionManager next, two of its four transformations should be completely unnecessary these days (changing pre-1.3.78 Token.propertyMap to Token.propertyCI, and removing pre-1.3.75 <exportInfo>). The third (stripping .dat extensions from asset filenames) doesn't actually do XML hacking, and also doesn't need anything so involved as ModelVersionManager. And the final one (changing <hasSight>false to <hasSight>true) we should be able to do after deserializing instead of before it, avoiding the final XML hack there.

@Azhrei
Copy link
Member

Azhrei commented May 11, 2023

I’m not sure we should strip old token support, even very old. I’ve got a bunch of tokens (with properties) that I created for campaigns a long time ago and it would be really nice to not have to recreate all of them! (Hm, the framework they were used in may not even work now, though. Still…)

@kwvanderlinde
Copy link
Collaborator Author

To be clear, I am in no way suggesting we stop supporting these tokens. Actually I want the opposite, to better support them (we have a reputation to uphold 😉). My comment above is to state that these particular transformations are either unnecessary or can be achieved another way, all while still supporting old tokens/campaigns.

Also I'm a wrong on the one. propertyMap -> propertyMapCI is more subtle than it seems ...

@bubblobill
Copy link
Collaborator

@kwvanderlinde don't suppose you removed all the i18n strings for this?

@kwvanderlinde
Copy link
Collaborator Author

Looks like I did not remove them. Will try have a look at that soonish.

@kwvanderlinde
Copy link
Collaborator Author

Closing this off as 1.15 is around the corner already. New issues can be opened if needed.

Menu now looks like this:
campaign-export-removed

@github-project-automation github-project-automation bot moved this from Needs Testing to Done in MapTool 1.14.0 Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-maintenance Adding/editing javadocs, unit tests, formatting.
Projects
Status: Done
Development

No branches or pull requests

4 participants