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> 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> 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
>>
>>
>>
