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

Ensure Unicode strings are converted when compiling with Unicode enabled #3301

Merged
merged 2 commits into from
Jul 4, 2023

Conversation

A-j-K
Copy link
Contributor

@A-j-K A-j-K commented Jul 4, 2023

Description

Qt6 appears to have introduced Unicode compilations. Type casting is insufficient and a full byte conversion is required.

Fixes #2934

Screenshots (if appropriate):

N/A

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update

How Has This Been Tested?

This was tested using my SkyWatcher SynScan Handcontroller AZ-GTiX Mount.

Note, this fix was at the serial port level and therefore I believe will address any mount that uses a serial port and not just my own SkyWatcher mount.

Test Configuration:

  • Operating system: Windows 11 Qt6 6.3.4

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [n/a] I have made corresponding changes to the documentation (header file)
  • [ n/a] I have updated the respective chapter in the Stellarium User Guide
  • My changes generate no new warnings
  • [ n/a] I have added tests that prove my fix is effective or that my feature works
  • [ n/a] New and existing unit tests pass locally with my changes
  • [ n/a] Any dependent changes have been merged and published in downstream modules

@10110111
Copy link
Contributor

10110111 commented Jul 4, 2023

Wouldn't it be better to always use Unicode-enabled CreateFileW, regardless of Qt version and UNICODE macro, instead of all this useless conditional compilation?

@10110111
Copy link
Contributor

10110111 commented Jul 4, 2023

Also, QString::fromUtf8("...").constData() already provides a sequence of UTF-16 code units, so no need to create an std::string and convert via codecvt.

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 4, 2023

Wouldn't it be better to always use Unicode-enabled CreateFileW, regardless of Qt version and UNICODE macro, instead of all this useless conditional compilation?

I currently don't have a system that can build a Qt5 version so my intention was to make changes that didn't disturb the build flow. In other words, I did what one would do in just 10 minutes to make it work ;)

Quite happy to rework it that way but I'm blind to other builds to test it all.

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 4, 2023

Also, QString::fromUtf8("...").constData() already provides a sequence of UTF-16 code units, so no need to create an std::string and convert via codecvt.

As I said previously, this is what I did in "10 minutes" just to get it working. We can improve on it if you desire.

@10110111
Copy link
Contributor

10110111 commented Jul 4, 2023

I currently don't have a system that can build a Qt5 version

You can use the AppVeyor build that's configured in .appveyor.yml in the source tree root. Prepending [publish] in commit messages produces binary artifacts. You should choose the "patch" one (which only installs stellarium.exe), otherwise if you download the full installer several times, you'll run over download limit.

Quite happy to rework it that way

Yes, please do. I really dislike how complicated the code is becoming. Even before this change the code looked needlessly complicated. So please make it simple, there's not a single reason to use 8-bit legacy code pages with Qt where QString already stores data in UTF-16.

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 4, 2023

@10110111 ok, I will look into that this evening as I am at work at the moment. Thanks for the feedback.

@gzotti
Copy link
Member

gzotti commented Jul 4, 2023

You can use the Qt MaintenanceTool in your Qt directory to add Qt5.15.2 (make sure to add QtScript and the other required additional libraries). Then in QtCreator, select the Kit for Qt5.15.2 and in the cmake configuration, make sure to set ENABLE_QT6 to false/0. You may need to set Qt5 libs instead of Qt6 into PATH as well.

+1 for simpler code. Note that most of the telescope plugin code roots in Qt4 times. I did the Qt6 ifdefs and was happy I could compile it, but had no hardware to test. If there are functions that work on both, Qt5 and Qt6, it's preferrable. Methods introduced after Qt5.12 would still be problematic, though, as the Qt5 releases are AFAIK still made with 5.12.

@alex-w is Qt5.9 still used somewhere, or could we raise at least to 5.12?

@alex-w
Copy link
Member

alex-w commented Jul 4, 2023

Qt5.9 still used somewhere, or could we raise at least to 5.12?

No, we can update requirements up to Qt 5.12

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 4, 2023

@gzotti @10110111

The previous commit now removes Qt6 conditional compilation and uses QString as it was meant to be as suggested.

This commit was tested using:-

  1. Windows 11 + Qt6 (6.4.3)
  2. Windows 11 + Qt5 (5.15.2) (yes, I can now build both :) )

Copy link
Contributor

@10110111 10110111 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.

@alex-w alex-w added this to the 23.3 milestone Jul 4, 2023
@alex-w
Copy link
Member

alex-w commented Jul 4, 2023

Probably this PR fixed #3006 already

Copy link
Member

@alex-w alex-w left a comment

Choose a reason for hiding this comment

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

Can't test on hardware, but looks ok

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 4, 2023

Can't test on hardware, but looks ok

Was tested for Qt5 and Qt6 using SkyWatcher SynScan Handcontroller and AZ-GTiX mount.

Copy link
Member

@gzotti gzotti left a comment

Choose a reason for hiding this comment

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

If it did not work before, and works with this on actual hardware, sure it's OK for me!

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 4, 2023

If it did not work before, and works with this on actual hardware, sure it's OK for me!

This is the case. For Qt6 my SynScan Hand controller did not work but it did work for Qt5. It now works for both.

@gzotti gzotti merged commit 2a77c0e into Stellarium:master Jul 4, 2023
@A-j-K A-j-K deleted the 2934-serial-port-issue branch July 4, 2023 14:26
@gzotti
Copy link
Member

gzotti commented Jul 4, 2023

@A-j-K thanks a lot for this. Are you interested in fixing more of these telescope issues (https://github.com/Stellarium/stellarium/projects/20#card-87377309)? And even complete the emergency stop #2754?

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 4, 2023

@A-j-K thanks a lot for this. Are you interested in fixing more of these telescope issues (https://github.com/Stellarium/stellarium/projects/20#card-87377309)? And even complete the emergency stop #2754?

@gzotti I noticed the Project Board for the Telescope Plugin was somewhat weighty with issues. I'll take a look and see if there's anywhere I can add value.

@gzotti
Copy link
Member

gzotti commented Jul 4, 2023

It is possible that this fix has already solved a handful of them. OPs would need to test their gear now with a new build. It's a pity 23.2 just came out, so this will likely take months...

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 5, 2023

@alex-w Given that #3305 just happened would it be worth considering a point release? This situation is only going to get worse until that next minor release happens.

@gzotti I spent yesterday evening reading every issue in the Telescope Project board looking for places to add value. I found little to none based entirely on the situation you already know, having the hardware to test. However, I don't see a reason to stop looking. Next, I will review the code as I saw it mentioned that it's not async by design other than everything must be happening in the draw() loop (to some degree, I will know for sure when I actually look at the code itself). Once I have done a review and essentially reverse-engineered how it all works I will probably come forward with a proposal on the way forward.

@alex-w
Copy link
Member

alex-w commented Jul 5, 2023

@alex-w Given that #3305 just happened would it be worth considering a point release? This situation is only going to get worse until that next minor release happens.

I'll create a development snapshot for this weekend, and we'll get an answer for #3305

@gzotti
Copy link
Member

gzotti commented Jul 5, 2023

I also have just fixed another error around a message connection that was renamed in Qt5.15 and would never have worked on Qt6. Whatever reported issue this probably fixes I cannot say as we rarely receive useful logfiles (where this issue would have been visible). This is one of the big problems with the TC plugin: we would need ~30 devices to test on 3 OS and 2 CPU families, and 30 user-developers to debug and keep the program working with their own gear, not withdrawing to obsolete versions on the slightest hint of problem. So, any help is welcome, and esp. working fixes after deep reverse-engineering insights!

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 5, 2023

@gzotti I have to ask... are Qt5 versions still being built because there are issues with Qt6 builds? If that's the case then the "withdrawing to obsolete versions at the slightest hint of a problem" is baked into the current deployment strategy. If that's not the case what is the reasoning behind both Qt5 and Qt6 builds/distributions?

@gzotti
Copy link
Member

gzotti commented Jul 5, 2023

Qt5 builds are required on older systems:

  • 32bit versions. This also includes users of older instrumentation with 32bit-only ASCOM drivers.
  • Windows 7 systems
  • Windows systems with weak graphic subsystems, e.g. Intel Core-i of 1st/2nd generation are blacklisted by Qt and need to run ANGLE mode, which is no longer available with Qt6. And there are even weaker systems (Pentium P6000, Celeron, Atom, ...), things that users with higher demands may just mask out when shopping for new systems.

It seems many observers use such oldish, otherwise obsolete systems to drive their telescopes, or they are in regions of this planet where PCs don't rain from the sky, and even old, used PCs are a major investment. A 2008 PC with Win10 (works in most cases!) and ANGLE is still better than no PC. Qt5 based releases for Windows should IMHO be provided as long as Win10 is widely used (i.e., at least V25.4), and as long as the code differences are miniscule. On Linux, some LTS distros may still have no Qt6. And who knows what will happen when probably hundreds of millions systems will (at least officially) have to move from Win10 to Linux in late 2025?

After release of 1.0 reports about issues with the TC plugin came in. About every report that included version number indicated Qt6, which is why we recommend using Qt5-based builds (but not obsolete 0.21 builds!). This most likely indicates I messed up something while porting to Qt6, but was not able to identify what. People complained in some astro forums instead of reporting with us, and often declared to have reverted to older versions.

@10110111
Copy link
Contributor

10110111 commented Jul 5, 2023

Pentium6

What is this? Do you mean Pentium Pro, with the P6 microarchitecture? This won't even run Windows 7. Pentium 4? This one is usable with a discrete GPU.

On Linux, some LTS distros may still have no Qt6

They are irrelevant, their official Stellarium packages won't be upgraded anyway until OS release upgrade.

have to move from Win10 to Linux

No one will have, just as many haven't moved from Windows 7 to anything, because at least this may bring problems with driver support.

@gzotti
Copy link
Member

gzotti commented Jul 5, 2023

Pentium6

What is this? Do you mean Pentium Pro, with the P6 microarchitecture? This won't even run Windows 7. Pentium 4? This one is usable with a discrete GPU.

Whatever, maybe my memory fails with all these CPU names. A year ago I was asked for help with an older sub-Core-i laptop. I installed Win10, and Stellarium 0.22.1 worked in ANGLE mode. The thing serves the daily needs of an older gentleman. Then maybe it was Pentium-IV. Already then I associated "Pentium" with the 1990s. Now they seem to also call Atoms "Pentium". And I admit, I will never learn their "Lakes".

EDIT: It was likely a P6000.

Now, fit a discrete GPU into such an old laptop!

On Linux, some LTS distros may still have no Qt6

They are irrelevant, their official Stellarium packages won't be upgraded anyway until OS release upgrade.

We provide Ubuntu packages for 18.04LTS. Does it have Qt6?

have to move from Win10 to Linux

No one will have, just as many haven't moved from Windows 7 to anything, because at least this may bring problems with driver support.

Requote: "(at least officially)"

@alex-w
Copy link
Member

alex-w commented Jul 5, 2023

We provide Ubuntu packages for 18.04LTS. Does it have Qt6?

Sorry, but no, we do not provide packages for Ubuntu 18.04 LTS

@gzotti
Copy link
Member

gzotti commented Jul 5, 2023

I was referring to https://launchpad.net/~stellarium/+archive/ubuntu/stellarium-releases
image
OK, maybe 23.1 was the last then?

I have two Odroid SBCs which are stuck with 18.04LTS. On these I like to have the latest features available, but can only build with Qt5. I hope I could still point out: being able to build with Qt5 (even without OS package manager support) will be needed for a few more years.

@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Jul 5, 2023
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Hello @A-j-K!

Please check the fresh version (development snapshot) of Stellarium:
https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 5, 2023

Hello @A-j-K!

Please check the fresh version (development snapshot) of Stellarium: https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot

@alex-w

Installed Qt6 version snapshot stellarium-23.2-a69d242-qt6-win64.exe

The serial port works perfectly now (didn't before with release Qt6)

Tested with a SkyWatcher SynScan Pro Hand Controller and AZ-GTiX mount.

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 5, 2023

Hello @A-j-K!

Please check the fresh version (development snapshot) of Stellarium: https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot

@alex-w

Installed Qt5 version snapshot stellarium-23.2-a69d242-qt5-win64.exe

The serial port works perfectly but it did before with a release Qt5.

Tested with a SkyWatcher SynScan Pro Hand Controller and AZ-GTiX mount.

@A-j-K
Copy link
Contributor Author

A-j-K commented Jul 6, 2023

Qt5.9 still used somewhere, or could we raise at least to 5.12?

No, we can update requirements up to Qt 5.12

Isn't 5.12 End-of-Life (as of December 2021) and 5.15 is LTS?

https://wiki.qt.io/Main

@10110111
Copy link
Contributor

10110111 commented Jul 6, 2023

Isn't 5.12 End-of-Life (as of December 2021) and 5.15 is LTS?

We follow actual use in the OS distributions. E.g. in Ubuntu 20.04 LTS (which is supported till 2025) Qt 5.12 is still in use.

@alex-w alex-w removed the state: published The fix has been published for testing in weekly binary package label Sep 26, 2023
@github-actions
Copy link

Hello @A-j-K!

Please check the latest stable version of Stellarium:
https://github.com/Stellarium/stellarium/releases/latest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Telescop control : cannot open serial device
4 participants