Plasma Firewall

[Ivan Čukić]

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ć
ivan at cukic.co, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232  07AE 01C6 CE2B FF04 1C12