qxmpp-project / qxmpp

Cross-platform C++ XMPP client and server library
410 stars 197 forks source link

Rename OmemoEnvelope's 'setIsUsedForKeyExchange()' to 'setUsedForKeyExchange()' #468

Open melvo opened 1 year ago

melvo commented 1 year ago

PR check list:

codecov[bot] commented 1 year ago

Codecov Report

Base: 68.42% // Head: 68.42% // Decreases project coverage by -0.00% :warning:

Coverage data is based on head (9e5647c) compared to base (9f474d2). Patch coverage: 60.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #468 +/- ## ========================================== - Coverage 68.42% 68.42% -0.01% ========================================== Files 279 279 Lines 24306 24305 -1 ========================================== - Hits 16631 16630 -1 Misses 7675 7675 ``` | [Impacted Files](https://codecov.io/gh/qxmpp-project/qxmpp/pull/468?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project) | Coverage Δ | | |---|---|---| | [src/omemo/QXmppOmemoManager\_p.cpp](https://codecov.io/gh/qxmpp-project/qxmpp/pull/468/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project#diff-c3JjL29tZW1vL1FYbXBwT21lbW9NYW5hZ2VyX3AuY3Bw) | `5.00% <0.00%> (ø)` | | | [src/base/QXmppOmemoDataBase.cpp](https://codecov.io/gh/qxmpp-project/qxmpp/pull/468/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project#diff-c3JjL2Jhc2UvUVhtcHBPbWVtb0RhdGFCYXNlLmNwcA==) | `98.78% <100.00%> (ø)` | | | [tests/qxmppomemodata/tst\_qxmppomemodata.cpp](https://codecov.io/gh/qxmpp-project/qxmpp/pull/468/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project#diff-dGVzdHMvcXhtcHBvbWVtb2RhdGEvdHN0X3F4bXBwb21lbW9kYXRhLmNwcA==) | `98.24% <100.00%> (-0.01%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qxmpp-project)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

lnjX commented 1 year ago

there are probably other places where this is the case, can you also adjust them?

melvo commented 1 year ago

There are the following setters:

  1. QXmppMessage::setIsSpoiler()
  2. QXmppMessage::setIsFallback()
  3. QXmppPresence::setIsPreparingMujiSession()
  4. QXmppRegisterIq::setIsRegistered()
  5. QXmppRegisterIq::setIsRemove()
  6. QXmppRosterIq::Item::setIsApproved()
  7. QXmppRosterIq::Item::setIsMixChannel()

Changing 1, 2, 3 and 7 could confuse the users because it gets a different meaning. E.g., setSpoiler() could be confused with setting the actual spoiler. We should use a consistent naming convention.

lnjX commented 1 year ago

I'd like to stick to the Qt conventions as much as possible.

https://wiki.qt.io/API_Design_Principles#Naming_Boolean_Getters.2C_Setters.2C_and_Properties

It says:

At first I agreed that having a setter called setMixChannel() sounds weird, but the more I think about it, it sounds more and more reasonable to me. I think setMixChannel(true) is okay.

However, it's probably a good idea to use more explicit types such as enums or structs to avoid such situations. For example:

I'd suggest to:

  1. replace setIsSpoiler() with setSpoiler(optional<Spoiler>) + spoiler struct
  2. setIsFallback(): wait with replacing until the contents of the new version of the fallback indicator are implemented and then use a fallback struct/class
  3. remove is in QXmppPresence::setIsPreparingMujiSession(), QXmppRegisterIq::setIsRegistered(), QXmppRegisterIq::setIsRemove(), QXmppRosterIq::Item::setIsApproved()
  4. replace setIsMixChannel(bool) with setMixChannel(optional<MixChannel>) and a struct/class MixChannel { QString participantId; }.

It would be great if you could do 3. in this PR, the other points can be done later (if you don't want to do them I can do them too). :)