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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

melvo
Copy link
Contributor

@melvo melvo commented Sep 24, 2022

PR check list:

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

@codecov
Copy link

codecov bot commented Sep 24, 2022

Codecov Report

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

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%) ⬇️

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

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

@lnjX
Copy link
Member

lnjX commented Sep 24, 2022

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

@melvo
Copy link
Contributor Author

melvo commented Sep 26, 2022

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
Copy link
Member

lnjX commented Oct 2, 2022

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 lnjX force-pushed the master branch 10 times, most recently from 84168ac to e614dcc Compare February 4, 2024 20:17
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.

2 participants