[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: D15093: Add WireGuard capability.
From: Bruce Anderson <noreply () phabricator ! kde ! org>
Date: 2018-08-31 2:35:40
Message-ID: 2dca26c24356c0db540c4da176c3893c () localhost ! localdomain
[Download RAW message or body]
[Attachment #2 (text/plain)]
andersonbruce added a comment.
I've used KDE for years but this is the first time I've written code using Qt so it \
doesn't surprise me that I didn't use some of the preferred methods of doing things. \
I have a few questions below and hopefully you will have a little patience with me if \
any seem like stupid questions.
In D15093#315890 <https://phabricator.kde.org/D15093#315890>, @pino wrote:
> Misc notes:
>
> - please remove all the translations (i.e. `Name=[lang]` & `Comment[lang]` from \
desktop/json files, since KDE has an automatic system to handle them
Do the automatic translations get added into the desktop files themselves at some \
point of the build process? If not, why do all the rest of the VPN implementations \
have translations in them?
> - please use KConfig \
<https://api.kde.org/frameworks/kconfig/html/classKConfig.html> to read & write \
ini-like files, instead of doing everything manually > - the `PasswordField` class \
is already available as `libs/editor/widgets/passwordfield.h`, part of the \
`plasmanm_editor` library, so you do not need to copy it locally > - \
`wireguard_global.h` seems not used, so just drop it > - `nm-wireguard-service.h` \
has lots of `NM_OPENVPN_` keys that are not used > - as @lbeltrame said, please use \
`QHostAddress` to parse IP addresses > - as @lbeltrame said, hardcoding colors is a \
bad idea, and it will not play nice with user choice of different color schemes, or \
accessibility purposes
What I am trying to do here is to highlight fields that don't have valid entries in \
them. I did this by turning the background in the field red if it wasn't valid and \
returning it to default when it became valid. Is there a "KDE way" to do something \
like this or should I just leave everything with the default background and not give \
the user any immediate feedback that there is a problem?
> - a better option to validate an input in a line edit is to use the embedded \
validator options, see `QLineEdit::setInputMask` and `QValidator` > - the better \
option to edit a port number is `QSpinBox` restricted to 0-65536, instead of a line \
edit
For the one entry which specifies only a port number, the most common instance is \
to leave this field empty. In my quick read through of the QSpinBox docs I didn't see \
any way to do a mixed 'no entry'/number option so I will probably leave this as a \
line edit.
> - there is a mixture of `virtual` & `Q_DECL_OVERRIDE` for virtual methods; \
plasma-nm uses `override` exclusively
Can you please give explicit references to where the problem is? I am not familiar \
with how Q_DECL_OVERRIDE is used but I did look at how all the other VPN \
implementations did it and it looks like they use the exact same mix of `virtual` & \
`Q_DECL_OVERRIDE` as I used in WireGuard.
> - please do not hardcode sizes and fonts in UI files
I can understand that fonts shouldn't be specified and have removed them. What I \
was trying to do was to highlight the text in a couple of labels by making them a \
little bigger than the default font and making them bold. Is there a "KDE way" to \
highlight something like this? Maybe a way to say "this is a header" or similar?
> - there are various texts in UI files with double spaces between words, please \
simply them to a single space > - manually calling \
`KAcceleratorManager::manage(this);` is not needed, why were they added?
Again this is due to my inexperience with Qt. Is there some reason that WireGuard \
doesn't need this but all the other VPN implementations do?
REPOSITORY
R116 Plasma Network Management Applet
REVISION DETAIL
https://phabricator.kde.org/D15093
To: andersonbruce, #plasma, jgrulich, pino
Cc: K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, ragreen, Pitel, \
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
[Attachment #3 (text/html)]
<table><tr><td style="">andersonbruce added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: \
right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: \
#F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: \
inline-block; border: 1px solid rgba(71,87,120,.2);" \
href="https://phabricator.kde.org/D15093">View Revision</a></tr></table><br \
/><div><div><p>I've used KDE for years but this is the first time I've \
written code using Qt so it doesn't surprise me that I didn't use some of \
the preferred methods of doing things. I have a few questions below and hopefully you \
will have a little patience with me if any seem like stupid questions.</p>
<blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a \
href="https://phabricator.kde.org/D15093#315890" style="background-color: #e7e7e7; \
border-color: #e7e7e7; border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D15093#315890</a>, <a \
href="https://phabricator.kde.org/p/pino/" style=" border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@pino</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Misc notes:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">please remove all the translations (i.e. <tt \
style="background: #ebebeb; font-size: 13px;">Name=[lang]</tt> & <tt \
style="background: #ebebeb; font-size: 13px;">Comment[lang]</tt> from desktop/json \
files, since KDE has an automatic system to handle them</li> </ul></div>
</blockquote>
<p>Do the automatic translations get added into the desktop files themselves at some \
point of the build process? If not, why do all the rest of the VPN implementations \
have translations in them?</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: \
italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul \
class="remarkup-list"> <li class="remarkup-list-item">please use <a \
href="https://api.kde.org/frameworks/kconfig/html/classKConfig.html" \
class="remarkup-link" target="_blank" rel="noreferrer">KConfig</a> to read & \
write ini-like files, instead of doing everything manually</li> <li \
class="remarkup-list-item">the <tt style="background: #ebebeb; font-size: \
13px;">PasswordField</tt> class is already available as <tt style="background: \
#ebebeb; font-size: 13px;">libs/editor/widgets/passwordfield.h</tt>, part of the <tt \
style="background: #ebebeb; font-size: 13px;">plasmanm_editor</tt> library, so you do \
not need to copy it locally</li> <li class="remarkup-list-item"><tt \
style="background: #ebebeb; font-size: 13px;">wireguard_global.h</tt> seems not used, \
so just drop it</li> <li class="remarkup-list-item"><tt style="background: #ebebeb; \
font-size: 13px;">nm-wireguard-service.h</tt> has lots of <tt style="background: \
#ebebeb; font-size: 13px;">NM_OPENVPN_</tt> keys that are not used</li> <li \
class="remarkup-list-item">as <a href="https://phabricator.kde.org/p/lbeltrame/" \
style=" border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@lbeltrame</a> said, please use <tt \
style="background: #ebebeb; font-size: 13px;">QHostAddress</tt> to parse IP \
addresses</li> <li class="remarkup-list-item">as <a \
href="https://phabricator.kde.org/p/lbeltrame/" style=" border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@lbeltrame</a> said, hardcoding colors is a bad \
idea, and it will not play nice with user choice of different color schemes, or \
accessibility purposes</li> </ul></blockquote>
<p>What I am trying to do here is to highlight fields that don't have valid \
entries in them. I did this by turning the background in the field red if it \
wasn't valid and returning it to default when it became valid. Is there a \
"KDE way" to do something like this or should I just leave everything with \
the default background and not give the user any immediate feedback that there is a \
problem?</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: \
italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul \
class="remarkup-list"> <li class="remarkup-list-item">a better option to validate an \
input in a line edit is to use the embedded validator options, see <tt \
style="background: #ebebeb; font-size: 13px;">QLineEdit::setInputMask</tt> and <tt \
style="background: #ebebeb; font-size: 13px;">QValidator</tt></li> <li \
class="remarkup-list-item">the better option to edit a port number is <tt \
style="background: #ebebeb; font-size: 13px;">QSpinBox</tt> restricted to 0-65536, \
instead of a line edit</li> </ul></blockquote>
<p>For the one entry which specifies only a port number, the most common instance is \
to leave this field empty. In my quick read through of the QSpinBox docs I \
didn't see any way to do a mixed 'no entry'/number option so I will \
probably leave this as a line edit.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: \
italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul \
class="remarkup-list"> <li class="remarkup-list-item">there is a mixture of <tt \
style="background: #ebebeb; font-size: 13px;">virtual</tt> & <tt \
style="background: #ebebeb; font-size: 13px;">Q_DECL_OVERRIDE</tt> for virtual \
methods; plasma-nm uses <tt style="background: #ebebeb; font-size: \
13px;">override</tt> exclusively</li> </ul></blockquote>
<p>Can you please give explicit references to where the problem is? I am not familiar \
with how Q_DECL_OVERRIDE is used but I did look at how all the other VPN \
implementations did it and it looks like they use the exact same mix of <tt \
style="background: #ebebeb; font-size: 13px;">virtual</tt> & <tt \
style="background: #ebebeb; font-size: 13px;">Q_DECL_OVERRIDE</tt> as I used in \
WireGuard.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: \
italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul \
class="remarkup-list"> <li class="remarkup-list-item">please do not hardcode sizes \
and fonts in UI files</li> </ul></blockquote>
<p>I can understand that fonts shouldn't be specified and have removed them. \
What I was trying to do was to highlight the text in a couple of labels by making \
them a little bigger than the default font and making them bold. Is there a "KDE \
way" to highlight something like this? Maybe a way to say "this is a \
header" or similar?</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: \
italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul \
class="remarkup-list"> <li class="remarkup-list-item">there are various texts in UI \
files with double spaces between words, please simply them to a single space</li> <li \
class="remarkup-list-item">manually calling <tt style="background: #ebebeb; \
font-size: 13px;">KAcceleratorManager::manage(this);</tt> is not needed, why were \
they added?</li> </ul></blockquote>
<p>Again this is due to my inexperience with Qt. Is there some reason that WireGuard \
doesn't need this but all the other VPN implementations do?</p></div></div><br \
/><div><strong>REPOSITORY</strong><div><div>R116 Plasma Network Management \
Applet</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a \
href="https://phabricator.kde.org/D15093">https://phabricator.kde.org/D15093</a></div></div><br \
/><div><strong>To: </strong>andersonbruce, Plasma, jgrulich, pino<br /><strong>Cc: \
</strong>K900, anthonyfieroni, pino, lbeltrame, ngraham, plasma-devel, ragreen, \
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart<br \
/></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic