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

Fix use of deprecated APIs in Qt 5.15 #235

Merged
merged 1 commit into from
Nov 22, 2022
Merged

Fix use of deprecated APIs in Qt 5.15 #235

merged 1 commit into from
Nov 22, 2022

Conversation

rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Nov 21, 2022

This is necessary to build Antimony with Qt 5.15, older versions of Qt are being deprecated and removed by distributions.

@rnhmjoj rnhmjoj mentioned this pull request Nov 21, 2022
31 tasks
QMap<Node*, QPointF> inspector;
QMap<Datum*, QPointF> subdatum;
QMultiMap<Node*, QPointF> inspector;
QMultiMap<Datum*, QPointF> subdatum;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand these changes – QMap is still supported, as is operator[].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

QMap is still supported but the unite method (used here) has been deprecated and moved to this QMultiMap subclass, that handle multiple values per-key. The [] operator has been made private as they want you to use value() or replace() instead.

@mkeeter
Copy link
Owner

mkeeter commented Nov 21, 2022

This looks reasonable, except the usage of QMultiMap instead of QMap - can you explain what that's about / what errors you were seeing before those changes? It changes the semantics of the operations, so it's not a drop-in replacement.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Nov 21, 2022

It changes the semantics of the operations, so it's not a drop-in replacement.

I tried following the Qt guide regarding the deprecation and migrating the code from QMap to QMultiMap assuming multiple values per-key weren't allowed before. If this is not true, I don't how to port the code, because I'm not sure about what map[key] would do before, with multiple values.

@mkeeter
Copy link
Owner

mkeeter commented Nov 21, 2022

I don't believe I'm using multiple values per key here. The list of obsolete members doesn't include operator[].

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Nov 21, 2022

I don't believe I'm using multiple values per key here.

Ok, then it should be fine. By the way, I build Antimony with Qt 5.15 and tested it by opening some .sb files and playing around a bit: everything seems fine.

The list of obsolete members doesn't include operator[].

As I said in the comment above, I had to change QMap to QMultiMap because unite() has been moved into QMultiMap and deprecated in QMap.

@mkeeter
Copy link
Owner

mkeeter commented Nov 21, 2022

Aha, I missed your comment about unite.

I believe that it's only used to join two maps – and not for the multi-value behavior – so could be replaced here with

void CanvasInfo::unite(const CanvasInfo& other)
{
    for (const auto i: other.inspector) {
        inspector.insert(i);
    }
    for (const auto s: other.subdatum) {
        subdatum.insert(s);
    }
}

(untested)

It looks like operator[] is still public for QMap, so if you modify the CanvasInfo::unite implementation (and switch back to QMap), you could go back to using it.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Nov 22, 2022

I've found a simpler solution: I missed that 5.15 overloaded the insert() method to take another QMap: assuming that no duplicate keys exists it works exactly like unite().

@mkeeter mkeeter merged commit 8b805c6 into mkeeter:develop Nov 22, 2022
@mkeeter
Copy link
Owner

mkeeter commented Nov 22, 2022

Thanks!

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Nov 22, 2022

Thank you. I really appreciate the quick response even if the project is in low mantainenance mode.

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.

None yet

2 participants