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
- 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
- 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
- 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
- Should add m_currentBackend = nullptr; after delete
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
- cbegin/cend instead of begin/end:
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
- mark member functions that can be const as const
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
- !...isEmpty is not needed - adds.contains(":") covers !...isEmpty
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
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
