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

Damaged/malformed XML contents flushed by CR w/o notice #47

Open
flaser opened this issue Feb 22, 2024 · 1 comment
Open

Damaged/malformed XML contents flushed by CR w/o notice #47

flaser opened this issue Feb 22, 2024 · 1 comment
Labels
bug Something isn't working Longterm This might take awhile low priority

Comments

@flaser
Copy link

flaser commented Feb 22, 2024

Describe the bug
Currently if XML deserialization fails, CR will attempt to re-popoulate metadata from the file-name alone.
Since this counts as "new" metadata the ComicInfo is marked as dirty.
Depending on settings, the User can then commit this to the file w/o being aware of the potential data-loss.

Exact Steps to Reproduce

  1. I open a CBZ file in 7-zip and edit it's ComicInfo.xml
    • I rename an opening or closing tag
    • E.g. <Writer> ... <Writer> ==> <Writer> ... <Write>
  2. In CR I open the file and check the "Info" tab and can confirm no metadata is there (only stuff guessed from file-name).
  3. I save the data to file in some fashion
  4. I open the CBZ file with 7-zip and check the ComicInfo.xml - it only has data gleamed from the filename

Version/Commit (check the about page, next to the version, for the string between brackets):

Additional context
The User should be made explicitly aware of XML parsing issues.
I propose the following behavior:

  • If deserialization fails, meta-data controls should be greyed out
    (Maybe add an icon or another indicator for XML issue)
  • When the User pushes Apply start Dialog:
    • Warning: Parsing ComicInfo.xml failed. If you commit your changes, CR will discard all data in the file.
    • Yes/No
    • Tickbox: always discard
@flaser flaser added the bug Something isn't working label Feb 22, 2024
@flaser flaser changed the title Damaged/Malformed XML Contents Flushed by CR Damaged/malformed XML contents flushed by CR w/o notice Feb 22, 2024
@maforget
Copy link
Owner

You will need to catch the correct exception, right now it only throws a InvalidOperationException not really descriptive. The Inner Exception is better. Maybe instead of returning a null, create an empty ComicInfo, with a property that states the Error Message and an IsError or something.

You also need to differentiate, between the other catch a level up that also returns null or if the file doesn't exists at all. Then probably catch the SetInfo to set something there also, so it doesn't just return null or set all the data. Then also the check if there is the info saved as a sidecar (.xml beside the file) or in the NTFS alternate stream. Then prevent any changes to the file for this exact reason , in all the place where files are written (On Update, On Exit, On Export, etc). Then it might just be for files not in the DB, because if they are then it would ask you to update the file and problem would be fixed. Wire all this up in the UI also to prevent any changes.

All that for an hypothetical malformed XML to not loose any changes. There are also big chances that any software will also discard it. It's malformed, we shouldn't jump through hoops to prevent some data from being lost.

Even if we use something like XmlSerializer.CanDeserialize, the ComicInfo would be null also, so back to the original problem.

It's a valid issue, so I will keep this up, but I see it as low priority, because the chances it happens are low and if it does you loose a little bit of data. You also have the NTFS as a backup in that case. I will personally not check it more than needed.

@maforget maforget added Longterm This might take awhile low priority labels Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Longterm This might take awhile low priority
Projects
None yet
Development

No branches or pull requests

2 participants