Implement QListElement for QLine(F)

[SanderVocke]

QListElement was not implemented for these types yet.

Let me know if I need to do anything in particular to be able to contribute a PR, license/copyright-wise. - right now I just copied the header from other similar files and put my name there.

This change builds for me and I am able to use the new list types.

Note that I also added two convenience methods to QPainter (which exist in Qt too), for drawing these lists of lines.

[LeonMatthesKDAB]

Hi @SanderVocke and thank you for the contribution!

The code it and of itself is good.
We do actually have a generate.sh script inside cxx-qt-lib/src/core/qlist/ that we used to generate all of these files.
So instead of adding the files for QLine(F) manually, you should be able to simply add a line to that script and execute it.

Can you please change to using that?
As it would allow us to update the code far easier if we ever need to e.g. add a new function to the trait.

Regarding the licensing:
We don't have a CLA, so you shouldn't have to do anything license-wise.
As you're contributing to this repo you implicitly agree that your code will made open-source and dual-licensed as apache 2.0 and MIT.
If you edit the generate.sh script to generate these files, your name won't be in the SPDX-FileContributor list of the QLine/F files. But you could simply add yourself as a SPDX-FileContributor on the generate.sh script itself.
Having multiple contributors is not a problem.

[SanderVocke]

OK, switched to the auto-generated version and included the change to generate.sh.

[LeonMatthesKDAB]

@SanderVocke thank you for the follow-up. I'm happy to approve this.

CI is still failing due to an issue with the formatting though.
Can you please run rustfmt on the modified files.

[SanderVocke]

The formatting should be fixed now.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3bd62ab) to head (1938323).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1053   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           71        71           
  Lines        11928     11928           
=========================================
  Hits         11928     11928           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ahayzen-kdab]

So this fails with Qt 5 in CI, I think it fails due to the QVector vs QList situation ?

Eg in Qt 5 QVector and QList were different, but in Qt 6 QVector is an alias of QList.

QPainter::drawLines in Qt 5 used a QVector, so i think if you change the type to be a QVector it'll maybe work ?

So i think what you need to do is

• Keep the QList implementation of QLine(F)
• Add a QVector implementation of QLine(F)
• Change the QPainter bindings to use the QVector version

Then it'll work with Qt 5 and Qt 6

https://doc.qt.io/qt-5/qpainter.html#drawLines-1
https://doc.qt.io/qt-6/qpainter.html#drawLines-1

[SanderVocke]

Ah, before you approve the CI run - I still need to run this locally against Qt5.

[SanderVocke]

I tried this locally against Qt5 and it builds fine, but some tests fail. In most cases it seems to be tests that make a diff between two result files, and the differences it finds are purely whitespace.

Would you mind running the CI and having a look for yourself?

[ahayzen-kdab]

Locally it could be a difference in clang-format versions or something, but CI seems happy :-)

[SanderVocke]

@LeonMatthesKDAB Not to push, but could you have another look?

[LeonMatthesKDAB]

@SanderVocke sorry I missed your latest update.
Looks great now and can be merged.

Thank you for the contribution!

[ahayzen-kdab]

(Note CI is failing due to macOS changing clang-format versions, see discussion in #1083 )

[SanderVocke]

Cheers, so if I understand correctly it is a matter of waiting for the clang-format to be fixed, then updating this branch?

[ahayzen-kdab]

Yes, this is separate from your change, once it is fixed we'll rebase yours and it'll land :-)