Review Request: add support for network zones
Lamarque Vieira Souza
lamarque at kde.org
Fri Jun 22 14:16:48 UTC 2012
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105324/#review14997
-----------------------------------------------------------
backends/NetworkManager/connectiondbus.cpp
<http://git.reviewboard.kde.org/r/105324/#comment11791>
Some NetworkManager settings when empty strings must not be added to the dbus map or NetworkManager invalidates the connection. Is this the case for NM_SETTING_CONNECTION_ZONE? If so then you must check if it is empty and do not add it here if it is empty.
I cannot answer if that is the case for NM_SETTING_CONNECTION_ZONE because that seems to be a implicit rule not described in the NetworkManager specification. You must look into NetworkManager source code to check if it the case for NM_SETTING_CONNECTION_ZONE.
libs/ui/connection.ui
<http://git.reviewboard.kde.org/r/105324/#comment11796>
If possible could you keep the order of items in increase order? This is row 2, it should go after row 1, not before row 0.
libs/ui/connection.ui
<http://git.reviewboard.kde.org/r/105324/#comment11797>
You should use KComboBox instead of QComboBox.
libs/ui/connection.ui
<http://git.reviewboard.kde.org/r/105324/#comment11799>
If this is a system connection and the user does not have permission to edit it this should be changed to false in the source code, no?
libs/ui/connectionwidget.cpp
<http://git.reviewboard.kde.org/r/105324/#comment11792>
It is not advisabled to include entire Qt modules into the source code. Please add only the classes you need here.
libs/ui/connectionwidget.cpp
<http://git.reviewboard.kde.org/r/105324/#comment11798>
Code style, you should use { } like this:
if (...) {
...
} else {
...
}
There are other places that needs the same code style change.
- Lamarque Vieira Souza
On June 22, 2012, 9:54 a.m., Lukáš Tinkl wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105324/
> -----------------------------------------------------------
>
> (Updated June 22, 2012, 9:54 a.m.)
>
>
> Review request for Network Management and Lamarque Vieira Souza.
>
>
> Description
> -------
>
> NetworkManager has had support for FirewallD [1] and Network Zones [2]
> since [3].
>
> This patch adds one combo box that enables to change the network zone.
> This box is visible only when FirewallD is running.
>
> [1] https://fedorahosted.org/firewalld/
> [2] https://fedoraproject.org/wiki/Features/network-zones
> [3] http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=1c0aeb457546ac5e485ca21dfe23f1a009912701
>
> (on behalf of Jiri Popelka <jpopelka at redhat.com>)
>
>
> Diffs
> -----
>
> backends/NetworkManager/connectiondbus.cpp 4649a19
> libs/internals/connection.h 485b810
> libs/internals/connection.cpp 67f31bc
> libs/ui/connection.ui 7b9f873
> libs/ui/connectionwidget.h aac7050
> libs/ui/connectionwidget.cpp f67f157
>
> Diff: http://git.reviewboard.kde.org/r/105324/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Lukáš Tinkl
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120622/11de49cb/attachment.html>
More information about the kde-networkmanager
mailing list