<table><tr><td style="">jgrulich 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/D18394">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/D18394#445224" style="background-color: #e7e7e7;
          border-color: #e7e7e7;
          border-radius: 3px;
          padding: 0 4px;
          font-weight: bold;
          color: black;text-decoration: none;">D18394#445224</a>, <a href="https://phabricator.kde.org/p/enriquem/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@enriquem</a> wrote:</div>
<div style="margin: 0;
          padding: 0;
          border: 0;
          color: rgb(107, 116, 140);"><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/D18394#444795" style="background-color: #e7e7e7;
          border-color: #e7e7e7;
          border-radius: 3px;
          padding: 0 4px;
          font-weight: bold;
          color: black;text-decoration: none;">D18394#444795</a>, <a href="https://phabricator.kde.org/p/jgrulich/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@jgrulich</a> wrote:</div>
<div style="margin: 0;
          padding: 0;
          border: 0;
          color: rgb(107, 116, 140);"><ol class="remarkup-list">
<li class="remarkup-list-item">I'm not sure if the UI for openconnect tokens is correct, I think the QLineEdit for token secret should be on the same line, and you should probably use PasswordField instead? It can be our PasswordField widget from libs/editor/widgets/. Or it's not secret in the same sense as other secrets and it will not need to be saved by secret agent, like rest of passwords? I would also follow nm-connection-editor and make tokens options visible in the main UI, not under specific button.</li>
</ol></div>
</blockquote>

<p>a) I don't see any need for the QComboBox and theQLineEdit to be in the same line, but that's a matter of taste, not functionality.  Both fields are sort of independent: same key works with different OTP options.</p></div>
</blockquote>

<p>All forms in the kcm use "Label: input" so I would like all the options to follow same pattern.</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>b) I agree on the PasswordField, although this being an OTP it really does not matter if anyone sees it.</p></blockquote>

<p>Ok, then I guess leave it as it is now.</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>c) No need to save it. It is used and discarded</p></blockquote>

<p>Doesn't seem to be true, when I save token secret, I see it is saved in NetworkManager so in this case PasswordField should be used and there should be an option to store it either to NM or secret agent.</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>d) I tried putting all options in the main UI. This made the window too tall for the allocated space, so that resizing was necessary or the main window initial size ought be changed. It looked ugly to me. That's why I opted for a separate dialog. I can change it if you think it is important, but, again, it looks ugly to me.</p></blockquote>

<p>In that case use a different button label, something like "Token Authentication".</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/D18394">https://phabricator.kde.org/D18394</a></div></div><br /><div><strong>To: </strong>enriquem, jgrulich<br /><strong>Cc: </strong>pino, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart<br /></div>