Pass command line filenames to already running instance by frostasm · Pull Request #1258 · keepassxreboot/keepassxc

frostasm · 2017-12-07T08:21:59Z

Description

Implement drag&drop support in main form to open database files
Pass command line filenames to already running instance

Motivation and context

To simplify the opening of files when one copy of the program is running (if was enabled the option "Start only a single instance of KeePassXC")

How has this been tested?

Manually on Linux OS

Screenshots (if appropriate):

Types of changes

✅ New feature (non-breaking change which adds functionality)

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.
❌ I have added tests to cover my changes.

frostasm · 2017-12-10T14:56:21Z

@TheZ3ro maybe you have time to check this PR?

TheZ3ro · 2017-12-10T15:34:00Z

@frostasm yes of course :)

frostasm · 2017-12-16T18:11:16Z

@TheZ3ro @droidmonkey please check my changes :)

frostasm · 2017-12-22T21:10:58Z

I did not found how to use QTest to test drag&drop. I will be grateful for the advice.

phoerious · 2017-12-24T14:05:04Z

src/gui/Application.cpp

        if (config()->get("SingleInstance").toBool()) {
            // Attempt to connect to the existing instance
            QLocalSocket client;
            for (int i = 0; i < 3; i++) {


phoerious · 2017-12-24T14:16:10Z

src/gui/MainWindow.cpp

+        if (!kdbxFiles.isEmpty()) {
+            event->acceptProposedAction();
+        }
+        for(const QString &kdbxFile: kdbxFiles) {


kdbxFiles inside kdbxFilesFromUrls() has gone out of scope here, so reference count should be 1 again, but use asConst() around kdbxFiles nevertheless. Also add a space after for and move the & sign to the type name.

@phoerious It would be nice if someone of the core developers have added information about how to use range-based for loop with Qt containers to the CONTRIBUTING.md

Good article about Qt containers and C++11 range-based loops

Also add a space after for and move the & sign to the type name.

done

Perhaps we should do that. You need to some understanding of how reference counters and detaching work in Qt. It's not KeePassXC stuff though. It's general Qt knowledge.

Of course these knowledge relates to Qt. But I'm sure that not everyone who sends patches understands how Qt lists and range-based for loops works :(

That's what code reviews are for. :-)

phoerious · 2017-12-24T14:19:53Z

src/main.cpp

-        for (int ii = filenames.size()-1; ii >= 0; ii--) {
-            QString filename = filenames.at(ii);
+        const QStringList fileNames = config()->get("LastOpenedDatabases").toStringList();
+        for (const QString &filename: fileNames) {


Use asConst() and move & to the type name.

asConst() function not needed with constant containers.

Qt A Porting Guide to C++11 Ranged For-Loops - Avoiding Detaching

Ummm... I was pretty sure it was non-const when I wrote that comment, but I must have overlooked it.

phoerious · 2017-12-24T14:20:31Z

src/main.cpp

-    for (int ii=0; ii < args.length(); ii++) {
-        QString filename = args[ii];
+    const bool pwstdin = parser.isSet(pwstdinOption);
+    for (const QString &filename: fileNames) {


asConst() and & to type.

phoerious · 2017-12-24T14:23:38Z

src/gui/Application.cpp

+        client->waitForBytesWritten(WaitTimeoutMSec);
+        client->disconnectFromServer();
+    }  else {
+        client->deleteLater();


You should move this to the top and add a return false after this statement.

I did not understand what I should do :(

Just move deleteLater() to the top, so you return early. Having a larger if block and then a simple else condition at the end of the function is quite error-prone and I consider it bad style. Better do whatever is in the else block right away and return and then you do what used to be in the if block.

I rewrote function

frostasm · 2017-12-24T22:17:46Z

I am also thinking about adding a unit test to pass file names between running instances. But I don't know how to check the correctness, because the current implementation is based on QApplication class (which is a singleton). Perhaps it makes sense to move the implementation of the checking "SingleInstance" and communication between processes in a separate class?

phoerious · 2017-12-25T11:37:32Z

It's hard to do, also because it's quite platform-dependent and raises the question if a whole application can still be seen as a unit. Usually, you want your unit tests to be fine-granular and atomic. If you come up with some good ideas, feel free to implement them. If not, I think this is good enough to be merged.

TheZ3ro · 2018-01-03T13:37:03Z

@frostasm can you please rebase this? Thanks

frostasm · 2018-01-03T14:08:11Z

of course :)

frostasm · 2018-01-03T14:51:42Z

@TheZ3ro done

phoerious · 2018-01-03T15:03:22Z

Thanks!

michaellass · 2018-02-28T17:49:33Z

Unfortunately this reverted my change from #774 and causes recently opened databases to reopen in reversed order on next program startup. Is that necessary to achieve the purpose of this PR or an was it an oversight?

droidmonkey · 2018-03-01T00:24:34Z

@michaellass I was wondering why that was a problem again! Definitely an oversight.

frostasm force-pushed the pass-command-line-filenames-to-already-running-instance branch 2 times, most recently from 27daf8f to 4f97872 Compare December 8, 2017 21:02

frostasm force-pushed the pass-command-line-filenames-to-already-running-instance branch from 4f97872 to 7506df3 Compare December 16, 2017 17:03

frostasm force-pushed the pass-command-line-filenames-to-already-running-instance branch from 7506df3 to 8b5a810 Compare December 22, 2017 14:17

phoerious added this to the v2.3.0 milestone Dec 24, 2017

phoerious added core user interface labels Dec 24, 2017

phoerious requested changes Dec 24, 2017

View reviewed changes

frostasm force-pushed the pass-command-line-filenames-to-already-running-instance branch 3 times, most recently from 450e61c to 411769c Compare December 24, 2017 21:36

frostasm force-pushed the pass-command-line-filenames-to-already-running-instance branch from 411769c to b3f03ed Compare December 25, 2017 10:45

phoerious approved these changes Dec 25, 2017

View reviewed changes

frostasm added 3 commits January 3, 2018 16:12

Implement drag&drop support in main form to open database files

7b9b23b

Pass (using IPC) command line filenames to already running instance

165d664

Add test for drag and drop database files into main window

3720c5e

frostasm force-pushed the pass-command-line-filenames-to-already-running-instance branch from b3f03ed to 3720c5e Compare January 3, 2018 14:15

phoerious merged commit ffddcb5 into keepassxreboot:develop Jan 3, 2018

frostasm deleted the pass-command-line-filenames-to-already-running-instance branch January 3, 2018 15:04

droidmonkey mentioned this pull request Mar 1, 2018

Correct file open ordering regression #1572

Closed

Uh oh!

Conversation

frostasm commented Dec 7, 2017

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

frostasm commented Dec 10, 2017

Uh oh!

TheZ3ro commented Dec 10, 2017

Uh oh!

frostasm commented Dec 16, 2017

Uh oh!

frostasm commented Dec 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frostasm Dec 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frostasm commented Dec 24, 2017

Uh oh!

phoerious commented Dec 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheZ3ro commented Jan 3, 2018

Uh oh!

frostasm commented Jan 3, 2018

Uh oh!

frostasm commented Jan 3, 2018

Uh oh!

phoerious commented Jan 3, 2018

Uh oh!

michaellass commented Feb 28, 2018

Uh oh!

droidmonkey commented Mar 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

phoerious Dec 24, 2017 •

edited

Loading

phoerious Dec 24, 2017 •

edited

Loading

phoerious Dec 24, 2017 •

edited

Loading

frostasm Dec 25, 2017 •

edited

Loading

phoerious commented Dec 25, 2017 •

edited

Loading