qxmpp
qxmpp copied to clipboard
Rename OmemoEnvelope's 'setIsUsedForKeyExchange()' to 'setUsedForKeyExchange()'
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)>
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.
there are probably other places where this is the case, can you also adjust them?
There are the following setters:
-
QXmppMessage::setIsSpoiler()
-
QXmppMessage::setIsFallback()
-
QXmppPresence::setIsPreparingMujiSession()
-
QXmppRegisterIq::setIsRegistered()
-
QXmppRegisterIq::setIsRemove()
-
QXmppRosterIq::Item::setIsApproved()
-
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.
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)
(andsetSpoilerHint()
) could be replaced withsetSpoiler(optional<Spoiler>)
and astruct Spoiler { QString hint; };
; -
setMixChannel()
could be replaced withsetChatType(ChatType)
and anenum 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:
- replace
setIsSpoiler()
withsetSpoiler(optional<Spoiler>)
+ spoiler struct -
setIsFallback()
: wait with replacing until the contents of the new version of the fallback indicator are implemented and then use a fallback struct/class - remove
is
inQXmppPresence::setIsPreparingMujiSession()
,QXmppRegisterIq::setIsRegistered()
,QXmppRegisterIq::setIsRemove()
,QXmppRosterIq::Item::setIsApproved()
- replace
setIsMixChannel(bool)
withsetMixChannel(optional<MixChannel>)
and a struct/classMixChannel { 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). :)