<div dir="ltr"><div>Lots of nitpicks fixed, still missing some, I'll finish the rest today.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 22, 2020 at 6:10 PM Ivan Čukić <<a href="mailto:ivan.cukic@kde.org">ivan.cukic@kde.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Kai,<br>
<br>
<br>
This is something that Plasma has been missing for a long time. Thanks to <br>
everyone involved!<br>
<br>
<br>
A few comments from a quick run-through over kcm/core:<br>
<br>
# Style issues<br>
<br>
- Some enums use MACRO_STYLE, some NormalStyle<br>
<br>
- Some one-line if bodies in the same file have {}, some do not<br>
<br>
- Double plural interfaces_names <a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/firewallclient.cpp#L56" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/firewallclient.cpp#L56</a><br>
<br>
- Personal preference - error handling in the if instead of out. This:<br>
  if (!m_currentBackend) { return nullptr; }<br>
  return proper;<br>
instead of this:<br>
  if (m_currentBackend) { return proper; }<br>
  return nullptr;<br>
<br>
- Missing m_ prefix for member variables:<br>
<a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/profile.h#L108" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
profile.h#L108</a><br>
<br>
- s/getRule/ruleAt/ - we don't usually have 'get' in the API if we can avoid <br>
it: <a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/ifirewallclientbackend.h#L50" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
ifirewallclientbackend.h#L50</a><br>
<br>
- Could use a pipe through clang-format<br>
<br>
<br>
# Non-style issues:<br>
<br>
- Should be ||, not && in:<br>
<a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/rulelistmodel.cpp#L37" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
rulelistmodel.cpp#L37</a><br>
<br>
- Can be a const or a ref-to-const, no need for qAsConst after:<br>
<a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/firewallclient.cpp#L58" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
firewallclient.cpp#L58</a><br>
<br>
- Should add m_currentBackend = nullptr; after delete<br>
<a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/firewallclient.cpp#L248" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
firewallclient.cpp#L248</a><br>
(otherwise you get UB on 270 if no plugins have been found)<br>
<br>
- Logic-wise - it would be better to redesign (if possible) not to allow <br>
m_currentBackend == nullptr instead of asserting and checking in each function<br>
(for example, to show a UI that can not call anything in the backend is no <br>
backend is loaded)<br>
<br>
- Wrong order - sort m_profiles and then replace it with a new contents?<br>
<a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/ifirewallclientbackend.cpp#L36" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
ifirewallclientbackend.cpp#L36</a><br>
<br>
- cbegin/cend instead of begin/end:<br>
<a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/ifirewallclientbackend.cpp#L42" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
ifirewallclientbackend.cpp#L42</a> <br>
<br>
- [&name] instead of [name]<br>
<a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/ifirewallclientbackend.cpp#L43" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
ifirewallclientbackend.cpp#L43</a><br>
<br>
- mark member functions that can be const as const<br>
<a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/ifirewallclientbackend.h#L89" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
ifirewallclientbackend.h#L89</a><br>
<br>
- else if for chained checks of <a href="http://reader.name" rel="noreferrer" target="_blank">reader.name</a>() == something<br>
<a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/profile.cpp#L163" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
profile.cpp#L163</a><br>
<br>
- !...isEmpty is not needed - adds.contains(":") covers !...isEmpty<br>
<a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/rule.cpp#L46" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/core/<br>
rule.cpp#L46</a><br>
<br>
- Assigning long-lived parents is naughty: <a href="https://invent.kde.org/tcanabrava/plasma-firewall/-/blob/master/kcm/backends/ufw/ufwclient.cpp#L318" rel="noreferrer" target="_blank">https://invent.kde.org/tcanabrava/<br>
plasma-firewall/-/blob/master/kcm/backends/ufw/ufwclient.cpp#L318</a><br>
<br>
<br>
<br>
So much from me at this point.<br>
<br>
Cheers,<br>
Ivan<br>
<br>
<br>
<br>
-- <br>
dr Ivan Čukić<br>
<a href="mailto:ivan@cukic.co" target="_blank">ivan@cukic.co</a>, <a href="https://cukic.co/" rel="noreferrer" target="_blank">https://cukic.co/</a><br>
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12<br>
<br>
<br>
</blockquote></div>