[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