Added ip-tunnel settings according to https://developer.gnome.org/NetworkManager/stable/settings-ip-tunnel.html
Details
Diff Detail
- Repository
- R282 NetworkManagerQt
- Lint
Lint Skipped - Unit
Unit Tests Skipped
autotests/settings/CMakeLists.txt | ||
---|---|---|
31 | Can you please add the test in alphabetic order? | |
autotests/settings/iptunnelsettingtest.cpp | ||
79 ↗ | (On Diff #46310) | Use NetworkManager defines, do not define your own new defines, there is no reason for that. |
autotests/settings/iptunnelsettingtest.h | ||
2 ↗ | (On Diff #46310) | You should add yourself here. |
src/CMakeLists.txt | ||
229 | Please add this in alphabetic order. | |
src/settings/connectionsettings.cpp | ||
195 ↗ | (On Diff #46310) | IpTunnel setting is not part of Tun connection, it should be completely separated connection type. |
src/settings/iptunnelsetting.cpp | ||
27 ↗ | (On Diff #46310) | Coding style, missing indentation. This applies also for lines below. |
src/settings/iptunnelsetting.h | ||
29 | There is no reason to create your own defines, use just those NM_SETTING_IP_TUNNEL_PROPERTY_NAME. | |
57 | There are other modes, they are just not documented in the documentation, but can be found in the source code. See: | |
74 | Maybe turn this into QFlags? See: |
autotests/settings/iptunnelsettingtest.cpp | ||
---|---|---|
79 ↗ | (On Diff #46310) | Only IPv4Setting and IPv6Setting do that this way, other setting classes don't. |
src/settings/connectionsettings.cpp | ||
195 ↗ | (On Diff #46310) | You just add it the same way as for example this Tun connection type is addded. |
src/settings/iptunnelsetting.h | ||
74 | Check QFlags, it's similar to enum, but you can combine them. |
You added just new changes you did on top of your original review, you need to include full patch, also with previous changes.
autotests/settings/iptunnelsettingtest.cpp | ||
---|---|---|
81 ↗ | (On Diff #46380) | This seems to be wrong. |
src/settings/connectionsettings.cpp | ||
197 ↗ | (On Diff #46381) | You need to add also IPv4 and IPv6 settings. |
316 ↗ | (On Diff #46381) | Same here. |
src/settings/connectionsettings.h | ||
70 ↗ | (On Diff #46381) | IpTunnel is correct name. |
src/settings/iptunnelsetting.cpp | ||
25 ↗ | (On Diff #46381) | You also need to define NM_SETTING_IP_TUNNEL_FLAGS in case NM is older than 1.12. |
27 ↗ | (On Diff #46381) | Coding style still. |
src/settings/iptunnelsetting.h | ||
59 | Turn this into QFlags. You can check how this should be done for example in VlanSetting | |
74 | Coding style. It should be setFoo(const QString &foo), the '&' should be separated from QString by a space and not separated by a space from "key". Same for other methods below. |
autotests/settings/iptunnelsettingtest.h | ||
---|---|---|
2 ↗ | (On Diff #46384) | Here should be your name. |
src/settings/connectionsettings.h | ||
70 ↗ | (On Diff #46384) | Remove the comment. |
src/settings/iptunnelsetting.cpp | ||
27 ↗ | (On Diff #46310) | You still don't follow KDE/Qt coding style. Check other classes to see how the indentation should be done. |
src/settings/iptunnelsetting.cpp | ||
---|---|---|
37 ↗ | (On Diff #46386) | Still misses indentation and code below as well. |
src/settings/iptunnelsetting.h | ||
29 | This shouldn't be commented out. | |
46 | There is no default mode, the first one should be Unknown. Also you maybe want to assign values to them? Like: enum Mode { Unknown = NM_IP_TUNNEL_MODE_UNKNOWN, Ipip = NM_IP_TUNNEL_MODE_IPIP etc. } | |
47 | The flags don't seem to use full name. For example NM_IP_TUNNEL_FLAG_IP6_IGN_ENCAP_LIMIT should perhabs be IgnEncapLimit. |