> On Jan. 2, 2013, 10:11 p.m., Lamarque Vieira Souza wrote:
> > libs/internals/settings/ipv6.cpp, line 14
> > <http://git.reviewboard.kde.org/r/108064/diff/1/?file=103610#file103610line14>
> >
> > I think you should create a enum like there Ipv6Setting::EnumMethod
> > instead of using hardcoded integer values. That works like a documentation
> > about what are the possible values for mPrivacy.
>
> Ralf Jung wrote:
> Okay, will do.
> Would you prefer the enum to be three-state (like in the patch proposed
> for the Gnome applet [1]), or four-state including "Unknown", like in
> libnm-util? The behaviour of "Unknown" used to differ from "Disabled", but
> this was changed [2] and now I can not find any indication for a difference
> in behaviour in the sources. As I said above, the docs do not mention
> "Unknown" at all... but it is the default value.
>
> [1] http://bugzilla-attachments.gnome.org/attachment.cgi?id=217377
> [2]
> http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=af0eb9e7adf0732921ebe927408a41a36f56f72e
>
> Lamarque Vieira Souza wrote:
> Four-state, we can use "Unkwon" internally.
>
> Jan Grulich wrote:
> Take a look at my implementation of IPv6Privacy [1] [2]
>
> [1]
> http://quickgit.kde.org/?p=libnm-qt.git&a=blob&h=0f7719078556e8654a009803c61cc1904e886990&hb=971ec1ff8dd66a144c79c2ac61e5d483e90b1630&f=settings%2Fipv6.h
>
> [2]
> http://quickgit.kde.org/?p=libnm-qt.git&a=blob&h=b2568805fc907b605faa34ecb7d4822f393acfe1&hb=971ec1ff8dd66a144c79c2ac61e5d483e90b1630&f=settings%2Fipv6.cpp
I prefer to use Unknown instead of Undefined like in NetworkManager's source
code to make it easier to search both source codes. In line 466 of the second
link you use the -1 value directly instead of the enumerate. There are some
set* methods where the parameters can be const, like in setMethod,
setIgnoreAutoRoutes, setIgnoreAutoDns, setNeverDefault, setMayFail and
setPrivacy. There are some mismatches in code style (for instance in some
places you use "if () {" and in some other places "if () <line break> {". The
former is the correct code style. In some places you use "while(){", there
should be more white spaces in there, like "while () {". You should follow code
style similar to [1]. You can use the astyle command line described in [1] to
do most of the changes for you, just keep in mind that astyle breaks the
canonical form of the SIGNAL() and SLOT() macros, you should use normalize [2]
after using astyle to fix that. The rest of the patches are ok, good work.
[1] http://techbase.kde.org/Policies/Kdelibs_Coding_Style
[2] git://gitorious.org/qt/qtrepotools/util/normalize
- Lamarque Vieira
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108064/#review24518
-----------------------------------------------------------
On Jan. 1, 2013, 4:28 p.m., Ralf Jung wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108064/
> -----------------------------------------------------------
>
> (Updated Jan. 1, 2013, 4:28 p.m.)
>
>
> Review request for Network Management.
>
>
> Description
> -------
>
> This patch adds an option to the IPv6 configuration screen to change the IPv6
> Privacy Extension configuration.
>
> The option is implemented as documented at
> http://projects.gnome.org/NetworkManager/developers/api/09/ref-settings.html
> . However, that document says the default value is "-1", while it also says
> that only 0, 1 and 2 are valid for this option. Therefore, I chose 0 to be
> the default in the Knm::Ipv6Setting class, but the option is always put into
> the NM config map - even if it is zero.
>
>
> This addresses bug 312305.
> http://bugs.kde.org/show_bug.cgi?id=312305
>
>
> Diffs
> -----
>
> backends/NetworkManager/settings/ipv6dbus.cpp 8c846ab
> libs/internals/settings/ipv6.h 2eadf69
> libs/internals/settings/ipv6.cpp 590274b
> libs/ui/ipv6.ui 56a1248
> libs/ui/ipv6widget.cpp 36b6885
>
> Diff: http://git.reviewboard.kde.org/r/108064/diff/
>
>
> Testing
> -------
>
> I verified that whatever I configure in the applet ends up in the
> /etc/NetworkManager/system-connections files.
>
> I could not yet test whether the extensions are actually properly enabled and
> disabled because the network I am currently in does not use IPv6. However,
> next week I will be at university again where I should be able to test this.
>
>
> Thanks,
>
> Ralf Jung
>
>
_______________________________________________
kde-networkmanager mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kde-networkmanager
