Oculars: begin code cleanup

[treaves]

Description

This is the start of code cleanup. But as there is so much to do, it's going to take too long to do it at once. This needs to be merged into master before more work can cinque; I just lost a few days of work due to a bad merge from upstream.

[github-actions]

Thanks for adding your first pull request to Stellarium. If you have questions, please do not hesitate to contact us.

[alex-w]

Please check CI/Linux and CI/AppVeyor reports.

[gzotti]

Interesting new syntax, I have never seen the "auto XY() --> type" . Are there real benefits? Cannot say. Makes reading harder for me.

Do I understand I shall definitely no longer bother with #1521. Fine for me, as long as our intended functionality does not suffer:

• separate view settings (store+reconfigure a few visibility settings on activation of Ocular or Sensor)
• optional auto-estimate of telescopic visibility conditions
• store current state of those and restore the cached main view settings on exit to main view.

[treaves]

Please check CI/Linux and CI/AppVeyor reports.

Yes, those build setups are broken. Not the code.

[treaves]

Interesting new syntax, I have never seen the "auto XY() --> type" . Are there real benefits? Cannot say. Makes reading harder for me.

Do I understand I shall definitely no longer bother with #1521. Fine for me, as long as our intended functionality does not suffer:

• separate view settings (store+reconfigure a few visibility settings on activation of Ocular or Sensor)

• optional auto-estimate of telescopic visibility conditions

• store current state of those and restore the cached main view settings on exit to main view.

There are benefits to the trailing returns.

Yes, I intend to do that functionality and more.

[axd1967]

Interesting new syntax, I have never seen the "auto XY() --> type" . Are there real benefits? Cannot say. Makes reading harder for me.

• https://stackoverflow.com/a/11215414/605463
• https://community.ibm.com/community/user/ibmz-and-linuxone/blogs/fang-lu2/2020/03/24/introduction-to-the-c11-feature-trailing-return-types?lang=en
• https://www.ibm.com/docs/en/zos/2.2.0?topic=declarators-trailing-return-type-c11

[alex-w]

Please check CI/Linux and CI/AppVeyor reports.

Yes, those build setups are broken. Not the code.

Syntax errors in the code are not code problem. Nice!

[alex-w]

Do you really need to use macro Q_DISABLE_COPY_MOVE(Class)?

[treaves]

Do you really need to use macro Q_DISABLE_COPY_MOVE(Class)?

I'll looks at the two separate macros tomorrow. But, yes, they need to be used an any subclass of QObject. And to your other comment, the two build failures are not do to issues in my code; you don't use a semicolon at the end of a macro. Is that why you suggest using the two macros?

[alex-w]

I'll looks at the two separate macros tomorrow. But, yes, they need to be used an any subclass of QObject.

For what? Which reasons for use this macros?

And to your other comment, the two build failures are not do to issues in my code; you don't use a semicolon at the end of a macro. Is that why you suggest using the two macros?

Just forget about 2 macros. You use macros, which was introduced in Qt 5.13 and now 2/3 of CI instances and all instances for building binary packages are broken.

[treaves]

I'll looks at the two separate macros tomorrow. But, yes, they need to be used an any subclass of QObject.

For what? Which reasons for use this macros?

And to your other comment, the two build failures are not do to issues in my code; you don't use a semicolon at the end of a macro. Is that why you suggest using the two macros?

Just forget about 2 macros. You use macros, which was introduced in Qt 5.13 and now 2/3 of CI instances and all instances for building binary packages are broken.

Ah. I did not see they were introduced so recently. They are needed because of the rule of three/five/zero. I'll replace them for the time being.

[lgtm-com]

This pull request introduces 28 alerts and fixes 1 when merging 1bb364a into 5f3aaa6 - view on LGTM.com

new alerts:

• 28 for Resource not released in destructor

fixed alerts:

• 1 for FIXME comment

[gzotti]

You can test for Qt versions and include/define what's possible. At some point we will have to step up to 5.15 and replace QtScript usage, before we can prepare further moving on to Qt6.

[treaves]

You can test for Qt versions and include/define what's possible. At some point we will have to step up to 5.15 and replace QtScript usage, before we can prepare further moving on to Qt6.

Ya, that's what I pushed earlier.

[treaves]

Do you really need to use macro Q_DISABLE_COPY_MOVE(Class)?

Did you see I put this into a guard?

[lgtm-com]

This pull request introduces 28 alerts and fixes 1 when merging 73043f6 into 5f3aaa6 - view on LGTM.com

new alerts:

• 28 for Resource not released in destructor

fixed alerts:

• 1 for FIXME comment

[alex-w]

Well, it works.

[alex-w]

All changes extracted to https://github.com/Stellarium/stellarium/tree/oculars-ng branch