Browser integration settings rework and KeePassHTTP deprecation #1392

phoerious · 2018-01-17T01:35:57Z

Description

This patch makes the following changes to the browser integration settings:

Reword and reorder options to make them easier to understand and more consistent
Move browser checkboxes to "General" tab to make them first-class options (will avoid user questions later why integration isn't working)
Hide unimplemented settings
Add "Browse..." button to proxy location setting

Furthermore, it deprecates KeePassHTTP and the WITH_XC_HTTP CMake flag and introduces a new WITH_XC_NETWORKING flag which separates general network access code from KeePassHTTP browser integration. Setting WITH_XC_HTTP implies WITH_XC_NETWORKING.

A CMake warning is issued when a user compiles KeePassXC with the deprecated WITH_XC_HTTP flag. Another (one-time) warning is shown to the user upon opening a database if KeePassHTTP is enabled in the settings. This warning links to a migration guide on our website which is yet to be written.

As a last change, the global message widget was redesigned a little. It now has proper margins and the warning background color is a little friendlier (although still not perfect, contrast is hard to get right with this combination of white text and blue links on yellow-ish background).

How has this been tested?

KeePassXC continues to compile and run fine with all four (three) possible combinations of the two flags WITH_XC_NETWORKING and WITH_XC_HTTP.

Screenshots (if appropriate):

(warning background color in the last screen shot is not quite accurate)

Types of changes

✅ Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
✅ My change requires a change to the documentation and I have updated it accordingly.

phoerious · 2018-01-17T09:30:05Z

I wonder if we should also rename keepassxc-browser into KeePassXC-Browser. It feels rather weird to ship something with our branding that has a different spelling.

TheZ3ro · 2018-01-17T09:46:46Z

src/browser/BrowserOptionDialog.cpp

+
+void BrowserOptionDialog::showProxyLocationFileDialog()
+{
+#ifdef Q_OS_WIN


We are using #if defined(Q_OS_WIN) elsewhere, we should stick with one notation

Well, we have both. defined() is necessary of you combine different conditions. I can change it, though.

Nah, fine by me.
But at some point we should convert every ifdef to if defined IMHO

TheZ3ro · 2018-01-17T09:49:49Z

src/browser/BrowserOptionDialog.cpp

+#else
+    QString fileTypeFilter(tr("Executable Files (*.*)"));
+#endif
+    auto proxyLocation = QFileDialog::getOpenFileName(this, tr("Select custom proxy location"),


Please use FileDialog::getOpenFileName()

I must admit I forgot about that class. But I don't think I should use it here. The main purpose of FileDialog is to remember the last open/save location. We don't want that here. In fact, it would be very annoying if the next time I click "Open Database" the selected directory is the KeePassXC install folder.

You can use FileDialog::setNextForgetDialog() before calling getOpenFileName() to not save the final destination, and you can specify the dir argument so you won't be placed in the LastDir.

Using our FileDialog implementation helps with future tests (since implements nextFileName), fixes focus on macOS and if in the future we will modify something that will get propagated here as well

I set a few additional flags for the QFileDialog, which can't be set via the static method, so I had to change this to a custom instantiation of QFileDialog. That makes it impossible to use FileDialog here. I therefore added the extra activateWindow() fix and would suggest to postpone this to a refactor of FileDialog, during which we make it a subclass of QFileDialog.

TheZ3ro · 2018-01-17T09:51:47Z

src/gui/AboutDialog.cpp

+    extensions += "\n- Browser Integration";
+#endif
+#ifdef WITH_XC_HTTP
+    extensions += "\n- Legacy Browser Integration (KeePassHTTP)";
 #endif


What about WITH_XC_NETWORKING?

Yeah, agreed. This should be added here.

Funnily, these all use #ifdef 😆

I saw that too after commenting about if defined 🤣

#ifdef is much preferred, IMO. #if defined( ) is annoying to read

droidmonkey · 2018-01-18T01:20:58Z

src/gui/AboutDialog.cpp

+    extensions += "\n- Browser Integration";
+#endif
+#ifdef WITH_XC_HTTP
+    extensions += "\n- Legacy Browser Integration (KeePassHTTP)";
 #endif


#ifdef is much preferred, IMO. #if defined( ) is annoying to read

droidmonkey · 2018-01-18T01:27:45Z

src/gui/KMessageWidget.cpp

@@ -287,7 +287,7 @@ void KMessageWidget::setMessageType(KMessageWidget::MessageType type)
            bg1 = palette().highlight().color();
            break;
        case Warning:
-            bg1.setRgb(181, 102, 0); // values taken from kcolorscheme.cpp (Neutral)
+            bg1.setRgb(191, 126, 7); // values taken from kcolorscheme.cpp (Neutral)


I recommend the following code instead (lighter bg, dark fg) for warnings. I am also really not a fan of the gradient, its a little aggressive and out of place with the rest of the UI.

QColor bg0, bg1, bg2, border; QColor fg = palette().light().color(); switch (type) { case Positive: bg1.setRgb(0, 110, 40); // values taken from kcolorscheme.cpp (Positive) break; case Information: bg1 = palette().highlight().color(); break; case Warning: bg1.setRgb(214, 190, 8); fg = Qt::black; break; case Error: bg1.setRgb(191, 3, 3); // values taken from kcolorscheme.cpp (Negative) break; } // Colors bg0 = bg1.lighter(110); bg2 = bg1.darker(110); border = darkShade(bg1);

I think this whole widget should get a little redesign for 2.4.0. It served us well, but it needs a refresh.

droidmonkey · 2018-01-18T01:29:47Z

src/gui/MainWindow.cpp

@@ -77,7 +78,7 @@ class HttpPlugin: public ISettingsPage

    QString name() override
    {
-        return QObject::tr("Browser Integration");
+        return QObject::tr("Browser Integration (old)");


Recommend "Legacy Browser Integration", changing the first word makes it stand out more. We should also change the icon, I recommend a NEW icon for the new plugin, maybe something like this.

I wanted to name it like that, but it's too long and I know it's even longer in other languages. Depending on font size and DPI settings, stuff may get cut off. Which in the other hand, is another argument in favor of putting it first, but "old browser integration" sounded a little too naive to me.
Windows is special with its tiny font size. You could fit a novel there.

For the icon, I'd like to have a different one, too, but we don't really have one. The one you linked looks completely out of place. A new icon needs to be consistent with the rest of the Oxygen icons. Or we need a fitting new logo for KeepasXC-Browser which is not just the KeePassXC logo.

I meant "like" it, not exactly that icon. Here is another idea:

(Add the KeePassXC Logo in the background on the computer monitor) http://www.iconarchive.com/show/oxygen-icons-by-oxygen-icons.org/Apps-krfb-icon.html

droidmonkey · 2018-01-18T01:32:50Z

src/gui/MainWindow.cpp

+                                "This feature has been deprecated and will be removed in the future.<br>"
+                                "Please switch to keepassxc-browser instead! For help with migration,<br>"
+                                "visit our <a href=\"https://keepassxc.org/docs/keepassxc-browser-migration\">"
+                                "keepassxc-browser migration guide</a>.</p>"),


Remove most of the breaks in this message, it does not render well on Windows.

Same problem as above. I had it with only one break and it looked far better on Windows. But other systems, other font sizes. The text will increase the minimum window width quite significantly. On small screens it may overflow. I tried to enable word wrapping, but that kind of caused the widget layout to fall apart.

droidmonkey · 2018-01-18T01:33:29Z

src/gui/MainWindow.cpp

+                                "keepassxc-browser migration guide</a>.</p>"),
+                         MessageWidget::Warning, true, -1);
+
+    config()->set("Http/DeprecationNoticeShown", true);


Change this to a counter, we will want to show it at least three times. You could write "Final Notice" on the third showing if you want.

droidmonkey · 2018-01-18T01:37:31Z

src/http/OptionDialog.ui

+   <item>
+    <widget class="QLabel" name="deprecationNotice">
+     <property name="text">
+      <string>&lt;p&gt;&lt;b&gt;NOTE:&lt;/b&gt; KeePassHTTP has been deprecated and will be removed in the future.&lt;br&gt;Please switch to keepassxc-browser instead! For help with migration, visit our &lt;a href=&quot;https://keepassxc.org/docs/keepassxc-browser-migration&quot;&gt;keepassxc-browser migration guide&lt;/a&gt;.&lt;/p&gt;</string>


This notice needs some left padding or center and bold it. Remove the "NOTE:" part. Different color?

phoerious · 2018-01-19T19:40:45Z

@droidmonkey I don't really like the yellow tint you chose, it's too pale. I therefore put a little work into it myself and gave the whole widget a little visual refresh. I changed all the colors to friendlier and warmer shades, reduced gradient strength and border radius, changed the border colour to a darker tint of the background colour and composed the close icon with an alpha-masked overlay rectangle of the foreground colour.

The result looks like this:

As you can see, I replaced the deprecation notice inside the KeePassHTTP settings with a message widget as well. I removed the warning icon from the "danger" warning of the advanced settings, because it didn't go well together with the almost identical background colour.

I also added a warning counter, so the global notice is shown three times now. Lastly, I renamed the KeePassHTTP settings category to "Legacy Browser Integration"

- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]

phoerious added user interface build system feature: Browser labels Jan 17, 2018

phoerious added this to the v2.3.0 milestone Jan 17, 2018

phoerious requested review from a team January 17, 2018 01:35

phoerious force-pushed the feature/browser-settings-rework branch from a9cb360 to 2752860 Compare January 17, 2018 01:44

varjolintu approved these changes Jan 17, 2018

View reviewed changes

TheZ3ro reviewed Jan 17, 2018

View reviewed changes

phoerious force-pushed the feature/browser-settings-rework branch 2 times, most recently from a6d8a27 to 794b2d4 Compare January 17, 2018 12:01

TheZ3ro approved these changes Jan 17, 2018

View reviewed changes

phoerious force-pushed the feature/browser-settings-rework branch from 794b2d4 to 77314f4 Compare January 18, 2018 00:52

droidmonkey requested changes Jan 18, 2018

View reviewed changes

phoerious added 3 commits January 19, 2018 20:40

Rework browser integration settings

f2b3181

Deprecate KeePassHTTP and introduce WITH_XC_NETWORKING CMake option

f520a0f

Improve KMessageWidget visuals

30f77b0

phoerious force-pushed the feature/browser-settings-rework branch from b1c7592 to b95c21b Compare January 19, 2018 19:41

Show KeePassHTTP deprecation notice three times and use MessageWidget

a6fd52d

phoerious force-pushed the feature/browser-settings-rework branch from b95c21b to a6fd52d Compare January 19, 2018 19:44

droidmonkey approved these changes Jan 19, 2018

View reviewed changes

phoerious merged commit ee3ccf1 into develop Jan 19, 2018

phoerious deleted the feature/browser-settings-rework branch January 19, 2018 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Browser integration settings rework and KeePassHTTP deprecation #1392

Browser integration settings rework and KeePassHTTP deprecation #1392

phoerious commented Jan 17, 2018 •

edited

Loading

phoerious commented Jan 17, 2018

TheZ3ro Jan 17, 2018

phoerious Jan 17, 2018

TheZ3ro Jan 17, 2018

TheZ3ro Jan 17, 2018

phoerious Jan 17, 2018 •

edited

Loading

TheZ3ro Jan 17, 2018

phoerious Jan 17, 2018

TheZ3ro Jan 17, 2018

phoerious Jan 17, 2018

phoerious Jan 17, 2018

TheZ3ro Jan 17, 2018

droidmonkey Jan 18, 2018

droidmonkey Jan 18, 2018

droidmonkey Jan 18, 2018

phoerious Jan 18, 2018

droidmonkey Jan 18, 2018

phoerious Jan 18, 2018

phoerious Jan 18, 2018

droidmonkey Jan 18, 2018

droidmonkey Jan 18, 2018

phoerious Jan 18, 2018

droidmonkey Jan 18, 2018

droidmonkey Jan 18, 2018

phoerious commented Jan 19, 2018 •

edited

Loading

Browser integration settings rework and KeePassHTTP deprecation #1392

Browser integration settings rework and KeePassHTTP deprecation #1392

Conversation

phoerious commented Jan 17, 2018 • edited Loading

Description

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

phoerious commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious commented Jan 19, 2018 • edited Loading

phoerious commented Jan 17, 2018 •

edited

Loading

phoerious Jan 17, 2018 •

edited

Loading

phoerious commented Jan 19, 2018 •

edited

Loading