<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>