-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
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.
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. |
Forgot to ask, have you built with qt6 to check if it works? |
I'd rather that the master branch have these changes now than wait until the Qt 6 branch is merged
Looks unrelated to anything I did
No, but it builds with Qt 5 and the component that Qt 6 removed is no longer there, so it should be fine |
Ok then, would be better to merge master on qt6 once this is merged, then to remove the compat module from there. |
There was a problem hiding this 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:
- Linux:
iconv
orrecode
- Windows:
gc
(PowerShell) - Online: https://www.freeformatter.com/convert-file-encoding.html
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.
* 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
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 theQt5Compat
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 useQTextCodec
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.