[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: D15093: Add WireGuard capability.
From: Pino Toscano <noreply () phabricator ! kde ! org>
Date: 2018-08-31 5:03:08
Message-ID: 693ec9d618280449c4f28766578b37ee () localhost ! localdomain
[Download RAW message or body]
[Attachment #2 (text/plain)]
pino added a comment.
In D15093#317960 <https://phabricator.kde.org/D15093#317960>, \
@andersonbruce wrote:
> 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?
There is an automatic system that extracts translations from desktop \
files (and alike), and injects translations back.
>> - 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?
I mentioned it one point below:
>> - a better option to validate an input in a line edit is to use the \
embedded validator options, see `QLineEdit::setInputMask` and `QValidator` \
IOW, instead of validating the input //after// the user inputs it, do it \
//before//.
>> - 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.
IMHO having 0 as value for that is good; otherwise, a proper \
QIntValidator for the line edit restricts the input the user can insert, \
removing the need to do the validation manually.
>> - 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?
`wireguardwidget.h`, for example.
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.
See commit 111ac65ae79f1a777e3b4a6389e916f0dfccd35e \
<https://phabricator.kde.org/R116:111ac65ae79f1a777e3b4a6389e916f0dfccd35e>. \
It looks like you are developing against an old version of plasma-nm, or \
not on master anyway.
>> - 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?
Rather than "how can I make some text bigger", the question is "what do \
you need to do". Since you are grouping widgets, then use a QGroupBox.
Oh, and that adds another thing to fix: `wireguard.ui` really needs a \
top-level layout, instead of hardcoding the size of the widgets...
>> - 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?
This was added by dbb4e2d5d47a6546014d433a1533d4ef4cfb7137 \
<https://phabricator.kde.org/R116:dbb4e2d5d47a6546014d433a1533d4ef4cfb7137>. \
Weird choice, I guess this can be left as it is.
REPOSITORY
R116 Plasma Network Management Applet
REVISION DETAIL
https://phabricator.kde.org/D15093
To: andersonbruce, #plasma, jgrulich, pino
Cc: acrouthamel, 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="">pino 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><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#317960" style="background-color: \
#e7e7e7; border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D15093#317960</a>, <a \
href="https://phabricator.kde.org/p/andersonbruce/" style=" border-color: \
#f1f7ff; color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@andersonbruce</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><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?</p></div> </blockquote>
<p>There is an automatic system that extracts translations from desktop \
files (and alike), and injects translations back.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; \
font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; \
background-color: #f8f9fc;"><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">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>
<p>I mentioned it one point below:</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; \
font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; \
background-color: #f8f9fc;"><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> \
</ul></blockquote></blockquote>
<p>IOW, instead of validating the input <em>after</em> the user inputs it, \
do it <em>before</em>.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; \
font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; \
background-color: #f8f9fc;"><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">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>
<p>IMHO having 0 as value for that is good; otherwise, a proper \
QIntValidator for the line edit restricts the input the user can insert, \
removing the need to do the validation manually.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; \
font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; \
background-color: #f8f9fc;"><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?</p></blockquote>
<p><tt style="background: #ebebeb; font-size: \
13px;">wireguardwidget.h</tt>, for example.</p>
<p>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>
<p>See commit <a href="https://phabricator.kde.org/R116:111ac65ae79f1a777e3b4a6389e916f0dfccd35e" \
style="background-color: #e7e7e7; border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: \
none;">111ac65ae79f1a777e3b4a6389e916f0dfccd35e</a>. It looks like you are \
developing against an old version of plasma-nm, or not on master \
anyway.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; \
font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; \
background-color: #f8f9fc;"><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>
<p>Rather than "how can I make some text bigger", the question is \
"what do you need to do". Since you are grouping widgets, then \
use a QGroupBox.</p>
<p>Oh, and that adds another thing to fix: <tt style="background: #ebebeb; \
font-size: 13px;">wireguard.ui</tt> really needs a top-level layout, \
instead of hardcoding the size of the widgets...</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; \
font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; \
background-color: #f8f9fc;"><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></blockquote>
<p>This was added by <a \
href="https://phabricator.kde.org/R116:dbb4e2d5d47a6546014d433a1533d4ef4cfb7137" \
style="background-color: #e7e7e7; border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: \
none;">dbb4e2d5d47a6546014d433a1533d4ef4cfb7137</a>. Weird choice, I guess \
this can be left as it is.</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>acrouthamel, 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