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

Remove usage of QTextCodec in Hydrogen Import plugin #7562

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

messmerd
Copy link
Member

@messmerd messmerd commented Oct 23, 2024

This takes us one step closer to Qt 6 support (#6614) since QTextCodec is one of the classes removed in Qt 6 and keeping it would require the use of the Qt5Compat module, which we'd like to avoid.

QTextCodec was only used by the HydrogenImport plugin when importing old Hydrogen files that were saved using TinyXML before it supported UTF-8. HydrogenImport would use QTextCodec to try to get the current encoding from the locale, and then use that as a best guess for interpreting the XML data in the unspecified encoding it was saved in. None of this was ever reliable, since the encoding of the computer that saved the Hydrogen file might not be the same as the computer running LMMS and importing that file.

There is no good solution here, so I decided to simply assume the old Hydrogen files are UTF-8 encoded. The worst that can happen is someone's ancient Hydrogen files containing non-ASCII text of some random encoding becomes mojibake'd after importing into LMMS, which is something that already could have happened.

I also cleaned up the Hydrogen Import source files a little bit.

QTextCodec was removed from Qt6 and is only available through the
Qt5Compat module.

QTextCodec was only used by the HydrogenImport plugin when importing old
Hydrogen files that were saved using TinyXML before it supported UTF-8.
HydrogenImport would use QTextCodec to try to get the current encoding
from the locale, and then use that as a best guess for interpreting the
XML data in the unspecified encoding it was saved in. None of this was
ever reliable, since the encoding of the computer that saved the
Hydrogen file might not be the same as the computer running LMMS and
importing that file.

There is no good solution here, so I decided to simply assume the old
Hydrogen files are UTF-8 encoded. The worst that can happen is someone's
ancient Hydrogen files containing non-ASCII text of some random encoding
becomes mojibake'd after importing into LMMS, which is something that
already could have happened.
@messmerd messmerd mentioned this pull request Oct 23, 2024
25 tasks
@Rossmaxx
Copy link
Contributor

Rossmaxx commented Oct 23, 2024

See the commit messages for details.

You could copy the message here for easier access.

Also, one thing i would suggest is that you open this PR against qt6 branch instead of master but doesn't really matter as we can merge master back in anyway.

Also might wanna look into the codefactor warnings.

@Rossmaxx
Copy link
Contributor

Forgot to ask, have you built with qt6 to check if it works?

@messmerd
Copy link
Member Author

@Rossmaxx

Also, one thing i would suggest is that you open this PR against qt6 branch instead of master but doesn't really matter as we can merge master back in anyway.

I'd rather that the master branch have these changes now than wait until the Qt 6 branch is merged

Also might wanna look into the codefactor warnings.

Looks unrelated to anything I did

Forgot to ask, have you built with qt6 to check if it works?

No, but it builds with Qt 5 and the component that Qt 6 removed is no longer there, so it should be fine

@Rossmaxx
Copy link
Contributor

Ok then, would be better to merge master on qt6 once this is merged, then to remove the compat module from there.

Copy link
Contributor

@michaelgregorius michaelgregorius left a comment

Choose a reason for hiding this comment

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

Looks good to me.

One additional option might be to point people to external and online file converters in the documentation, for example:

That way we put the "burden" of the conversion away from LMMS (which is accomplished with this PR) and to better suited external tools.

Another option might be to point people to these tools inside of the application in case of a failure to import the Hydrogen file.

Although it is a question anyway how often people will really run into these problems. If I understand correctly a user will have to import a very old Hydrogen file into an LMMS version which was built after this PR got merged. Sounds rather unlikely.

@messmerd messmerd merged commit e6776bc into LMMS:master Oct 23, 2024
11 checks passed
@messmerd messmerd deleted the qt6-remove-qt5compat branch October 23, 2024 17:17
mmeeaallyynn pushed a commit to mmeeaallyynn/lmms that referenced this pull request Oct 27, 2024
* Remove QTextCodec

QTextCodec was removed from Qt6 and is only available through the
Qt5Compat module.

QTextCodec was only used by the HydrogenImport plugin when importing old
Hydrogen files that were saved using TinyXML before it supported UTF-8.
HydrogenImport would use QTextCodec to try to get the current encoding
from the locale, and then use that as a best guess for interpreting the
XML data in the unspecified encoding it was saved in. None of this was
ever reliable, since the encoding of the computer that saved the
Hydrogen file might not be the same as the computer running LMMS and
importing that file.

There is no good solution here, so I decided to simply assume the old
Hydrogen files are UTF-8 encoded. The worst that can happen is someone's
ancient Hydrogen files containing non-ASCII text of some random encoding
becomes mojibake'd after importing into LMMS, which is something that
already could have happened.

* Clean up a little
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.

4 participants