[rekonq] Review Request 120794: Implement Unique Mode Properly in Rekonq

[David Narváez]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------

Review request for KDE Frameworks and rekonq.


Repository: rekonq


Description
-------

This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.

The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.


Diffs
-----

src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a

Diff: https://git.reviewboard.kde.org/r/120794/diff/


Testing
-------

1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)

Before this patch, nothing happens in steps 2 - 4. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly.


Thanks,

David Narváez

[Andrea Diamantini]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69131
-----------------------------------------------------------

Ship it!


Everything works as described! Many thanks David, your work here is really appreciated :)

- Andrea Diamantini

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 25, 2014, 5:26 a.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
Before this patch, nothing happens in steps 2 - 4. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly.
Thanks,
David Narváez

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69227
-----------------------------------------------------------


You're probably right that this is the first real port to QCLP+KDBusService. Let's make it good and then let's document the procedure.

Why not keep the parser instance and call process() or parse() again, if the option definitions are the same? QCLP supports this.

- David Faure

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 25, 2014, 5:26 a.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
Before this patch, nothing happens in steps 2 - 4. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly.
Thanks,
David Narváez

[David Narváez]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------

(Updated Oct. 28, 2014, 5:44 a.m.)


Review request for KDE Frameworks and rekonq.


Changes
-------

Rewrote the command line parser approach: now the parser is created in the Application object and the about info is populated in the main function. We lost the ability to query for the incognito and webapp options using QCommandLineOptions (now it uses the isSet(QString) method) but there's overall less code duplication and adding a new option would require changes in just one file. Good enough?


Repository: rekonq


Description
-------

This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.

The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.


Diffs (updated)
-----

src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a

Diff: https://git.reviewboard.kde.org/r/120794/diff/


Testing (updated)
-------

1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com

Before this patch, nothing happens in steps 2 - 6. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window).


Thanks,

David Narváez

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69243
-----------------------------------------------------------



src/application.h
<https://git.reviewboard.kde.org/r/120794/#comment48403>

should be const QStringList &arguments



src/application.cpp
<https://git.reviewboard.kde.org/r/120794/#comment48404>

that's definitely in the wrong place, if it ever gets activated. You want session restoration only in main, not every time activateRequested is called.



src/application.cpp
<https://git.reviewboard.kde.org/r/120794/#comment48405>

It's a bit confusing because QCLP::process() is not the same, and also because of the other processCmdLine method here. Maybe call this one handleCmdLine()? Sorry for nitpicking, but if we are going to replace this in most other apps, might as well be perfect :-)

parse = just the parsing
process = parse and handle the builtin options (in QLCP)
handle = handle the app options

As far as the slot is concerned, it's ok to say that Application::processCmdLine is parse + handle the app options, it's "builtin to Application". So I would rename this method, but leave the other one (processCmdLine(arguments)) as it is.



src/application.cpp
<https://git.reviewboard.kde.org/r/120794/#comment48409>

Note that you might want to use the cwd in order to resolve urls with it, so that "rekonq localfile.html" works.
This requires QUrl::fromUserInput(two args) from Qt 5.4 though, so probably too early to do right now, but you can at least add a comment, if not a #if QT_VERSION check.



src/main.cpp
<https://git.reviewboard.kde.org/r/120794/#comment48406>

why not move this to the Application constructor too? i.e. do all the setup of the parser in a single place?

I guess only the interaction with about stays here.

Alternatively (and because not all apps derive from QApplication), I would just leave it all in main (where it is expected, in most apps), and do app.setParser(&parser).

In addition to having all the setup in one place, the thing I'm trying to solve here is the "getCmdLineParser" method, which doesn't look so great in terms of encapsulation. getters/setters are always better than "returning a ref to an internal member variable".



src/main.cpp
<https://git.reviewboard.kde.org/r/120794/#comment48407>

app.connect --> QObject::connect.
You're using the 4-args connect, which is a static method.


- David Faure

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 5:44 a.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
Before this patch, nothing happens in steps 2 - 6. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window).
Thanks,
David Narváez

[David Narváez]

Post by David Narváez

src/application.cpp, line 785
<https://git.reviewboard.kde.org/r/120794/diff/2/?file=322403#file322403line785>
It's a bit confusing because QCLP::process() is not the same, and also because of the other processCmdLine method here. Maybe call this one handleCmdLine()? Sorry for nitpicking, but if we are going to replace this in most other apps, might as well be perfect :-)
parse = just the parsing
process = parse and handle the builtin options (in QLCP)
handle = handle the app options
As far as the slot is concerned, it's ok to say that Application::processCmdLine is parse + handle the app options, it's "builtin to Application". So I would rename this method, but leave the other one (processCmdLine(arguments)) as it is.

This is up for review precisely to get it nitpicked :)


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69243
-----------------------------------------------------------

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 5:44 a.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
Before this patch, nothing happens in steps 2 - 6. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window).
Thanks,
David Narváez

[David Narváez]

Post by David Narváez

src/application.cpp, line 789
<https://git.reviewboard.kde.org/r/120794/diff/2/?file=322403#file322403line789>
Note that you might want to use the cwd in order to resolve urls with it, so that "rekonq localfile.html" works.
This requires QUrl::fromUserInput(two args) from Qt 5.4 though, so probably too early to do right now, but you can at least add a comment, if not a #if QT_VERSION check.

Local URL handling is indeed broken, I added a TODO comment to work on it later, but added parameters that would be missing if I wanted to implement this properly in the handleCmdLine method


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69243
-----------------------------------------------------------

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 2 p.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
7. Open rekonq from the console pointing it to some local HTML file
Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken.
Thanks,
David Narváez

[David Narváez]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------

(Updated Oct. 28, 2014, 2 p.m.)


Review request for KDE Frameworks and rekonq.


Changes
-------

Addressed most of the issues raised by dfaure, except local URL handling which is still broken.


Repository: rekonq


Description
-------

This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.

The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.


Diffs (updated)
-----

src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a

Diff: https://git.reviewboard.kde.org/r/120794/diff/


Testing (updated)
-------

1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
7. Open rekonq from the console pointing it to some local HTML file

Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken.


Thanks,

David Narváez

[Andrea Diamantini]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69317
-----------------------------------------------------------



src/application.cpp
<https://git.reviewboard.kde.org/r/120794/#comment48461>

I guess cwd should be Q_UNUSED until it will be 'used' :)


- Andrea Diamantini

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 2 p.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
7. Open rekonq from the console pointing it to some local HTML file
Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken.
Thanks,
David Narváez

[Andrea Diamantini]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69318
-----------------------------------------------------------



src/application.cpp
<https://git.reviewboard.kde.org/r/120794/#comment48462>

It's not clear to me why you chose parse() instead of process() here.
Can you please argument this?


- Andrea Diamantini

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 2 p.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
7. Open rekonq from the console pointing it to some local HTML file
Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken.
Thanks,
David Narváez

[David Narváez]

Post by David Narváez

src/application.cpp, line 731
<https://git.reviewboard.kde.org/r/120794/diff/3/?file=322475#file322475line731>
It's not clear to me why you chose parse() instead of process() here.
Can you please argument this?

It would be equivalent to process() because the --help and --version arguments were already handled by the original application if they were present (i.e., the application would have died before requesting our activation).


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69318
-----------------------------------------------------------

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 29, 2014, 1:57 p.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
7. Open rekonq from the console pointing it to some local HTML file
Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken.
Thanks,
David Narváez

[David Narváez]

Post by David Narváez

src/application.cpp, line 731
<https://git.reviewboard.kde.org/r/120794/diff/3/?file=322475#file322475line731>
It's not clear to me why you chose parse() instead of process() here.
Can you please argument this?

It would be equivalent to process() because the --help and --version arguments were already handled by the original application if they were present (i.e., the application would have died before requesting our activation).

Moreover, if you were to use process() here, and external entity could "shoot your application dead" if it sends --help or --version parameters through the right D-Bus call. I guess this is an important point to mention in a porting guide.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69318
-----------------------------------------------------------

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 29, 2014, 1:57 p.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
7. Open rekonq from the console pointing it to some local HTML file
Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken.
Thanks,
David Narváez

[David Narváez]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------

(Updated Oct. 29, 2014, 1:57 p.m.)


Review request for KDE Frameworks and rekonq.


Changes
-------

Added Q_UNUSED where needed and removed the processCmdLine method since it has no real purpose now.


Repository: rekonq


Description
-------

This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.

The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.


Diffs (updated)
-----

src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a

Diff: https://git.reviewboard.kde.org/r/120794/diff/


Testing
-------

1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
7. Open rekonq from the console pointing it to some local HTML file

Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken.


Thanks,

David Narváez

[Andrea Diamantini]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69530
-----------------------------------------------------------

Ship it!


Seems good to me now.

- Andrea Diamantini

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 29, 2014, 1:57 p.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
7. Open rekonq from the console pointing it to some local HTML file
Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken.
Thanks,
David Narváez

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/#review69565
-----------------------------------------------------------

Ship it!


Looking nice now! Thanks.


src/main.cpp
<https://git.reviewboard.kde.org/r/120794/#comment48735>

This should pass QDir::currentPath() to the method, for later, when cwd is used in there ;)

(which also means the default value for cwd can be removed)


- David Faure

Post by David Narváez
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------
(Updated Oct. 29, 2014, 1:57 p.m.)
Review request for KDE Frameworks and rekonq.
Repository: rekonq
Description
-------
This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.
The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.
Diffs
-----
src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a
Diff: https://git.reviewboard.kde.org/r/120794/diff/
Testing
-------
1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
7. Open rekonq from the console pointing it to some local HTML file
Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken.
Thanks,
David Narváez

[David Narváez]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120794/
-----------------------------------------------------------

(Updated Nov. 2, 2014, 5:23 p.m.)


Status
------

This change has been marked as submitted.


Review request for KDE Frameworks and rekonq.


Repository: rekonq


Description
-------

This is my humble attempt to implement the Unique Mode properly. I have been trying to do this for the longest time in a way that avoids code duplication but I can't find a way to jump over all the hurdles these API impose. I tried learning from other ports from KUniqueApplication but a quick look at LXR shows there are plenty of applications that blindly ported to Unique Mode but didn't bother implementing activateRequested and the one I found that did was plasmawindowedcorona.cpp which does not need a QCommandLineParser, so the code duplication is less evident. At this point, I would like someone who knows about the QCommandLineParser + KDBusAddons dance to look at this and tell if it is reasonable or not.

The current patch just makes it possible to open several Rekonq applications. It does not do the right thing when a Rekonq window is already open in the current activity and a user clicks a link elsewhere (step 4 in the Testing Done section) because it starts a brand new Rekonq window, but that's a different patch. It also does some funky thing asking you if you want to restore the previous session when nothing has crashed, I have to check that.


Diffs
-----

src/application.h 7ccd60d
src/application.cpp c7c297d
src/main.cpp 7592f7a

Diff: https://git.reviewboard.kde.org/r/120794/diff/


Testing
-------

1. Open one Rekonq window
2. Try opening Rekonq again
3. Try opeining Rekonq from a command line with some URLs
4. Assuming Reknoq is your default browser (why wouldn't it be?) click on a link somewhere (I click on the links at the title of the Konversation channels I am in)
5. Open rekonq from the console using rekonq --incognito
6. Open rekonq from the console using reknoq --webapp twitter.com
7. Open rekonq from the console pointing it to some local HTML file

Before this patch, nothing happens in steps 2 - 7. After a first version of this patch that does not avoid the QCommandLine parser if the argument list is not empty, the window opened at 1 crashes because the activateRequested signal passes an empty list of arguments - not even the binary name - so QCommandLine parser dies. With this patch, every step opens a new window properly, step 5 opens an incognito window and step 6 opens a webapp window (simple window). Step 7 opens the current working directory because local URL handling is broken.


Thanks,

David Narváez