I've got firewalld installed, yet I see an error asking me to install it
upon opening the KCM: https://i.imgur.com/qQzYLwH.jpg
Nate
On 9/17/20 5:20 AM, Tomaz Canabrava wrote:
People, I have spend a great deal of time fine tuning the Firewall, I
hope it's ready for a release.
On Wed, Jun 3, 2020 at 6:33 PM Tomaz Canabrava <tcanabr...@kde.org
<mailto:tcanabr...@kde.org>> wrote:
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
<mailto: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
<http://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 <mailto:i...@cukic.co>, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232 07AE 01C6 CE2B
FF04 1C12
