qxmpp icon indicating copy to clipboard operation
qxmpp copied to clipboard

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

Open melvo opened this issue 2 years ago • 4 comments

PR check list:

  • [x] Document your code
  • [x] Add \since QXmpp 1.X, QXMPP_EXPORT
  • [x] Fix doxygen warnings (see log when building with -DBUILD_DOCUMENTATION=ON)
  • [x] Update doc/doap.xml
  • [x] Add unit tests
  • [x] Format the code: Run clang-format -i src/<edited-file(s)> tests/<edited-file(s)>

melvo avatar Sep 24 '22 10:09 melvo

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
@@            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 Coverage Δ
src/omemo/QXmppOmemoManager_p.cpp 5.00% <0.00%> (ø)
src/base/QXmppOmemoDataBase.cpp 98.78% <100.00%> (ø)
tests/qxmppomemodata/tst_qxmppomemodata.cpp 98.24% <100.00%> (-0.01%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

codecov[bot] avatar Sep 24 '22 10:09 codecov[bot]

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

lnjX avatar Sep 24 '22 10:09 lnjX

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.

melvo avatar Sep 26 '22 13:09 melvo

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:

  • nouns shouldn't have is for the getter at all, except it would be misleading. (e.g. isMixChannel() instead of mixChannel())
  • setters should never have is in it

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:

  • setSpoiler(bool) (and setSpoilerHint()) could be replaced with setSpoiler(optional<Spoiler>) and a struct Spoiler { QString hint; };;
  • setMixChannel() could be replaced with setChatType(ChatType) and an enum ChatType { User, MixChannel } (in this case this approach could lead to issues later tough, e.g. if the channel is also a MUC and MIX2 chat and that should also be represented)

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). :)

lnjX avatar Oct 02 '22 15:10 lnjX