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