Discussion:
Review Request: Better behaviour of the bookmarks runner regarding Firefox
Jan Gerrit Marker
2010-06-20 16:40:05 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4414/
-----------------------------------------------------------

Review request for Plasma.


Summary
-------

Corrects the behavior of the bookmarks runner regarding Firefox:
1. Firefox intern bookmarks (like the "Recently used bookmarks" folder) are not shown
2. Only bookmarks with a non empty name and url are shown
3. The profile written to the config file is updated if it does not exist anymore

Additionally I polished the Firefox regarding method.
I did not commit directly because I was not sure whether it is allowed at this point of the KDE SC development.


Diffs
-----

/trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp 1140335

Diff: http://reviewboard.kde.org/r/4414/diff


Testing
-------

I have tested it with current trunk - everything seems to be ok.


Thanks,

Jan Gerrit
Marco Martin
2010-06-20 16:52:52 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4414/#review6197
-----------------------------------------------------------


i think the patch is ok, i am concerned too about committing so late a change that is not exactly trivial

- Marco
Post by Jan Gerrit Marker
-----------------------------------------------------------
http://reviewboard.kde.org/r/4414/
-----------------------------------------------------------
(Updated 2010-06-20 16:40:04)
Review request for Plasma.
Summary
-------
1. Firefox intern bookmarks (like the "Recently used bookmarks" folder) are not shown
2. Only bookmarks with a non empty name and url are shown
3. The profile written to the config file is updated if it does not exist anymore
Additionally I polished the Firefox regarding method.
I did not commit directly because I was not sure whether it is allowed at this point of the KDE SC development.
Diffs
-----
/trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp 1140335
Diff: http://reviewboard.kde.org/r/4414/diff
Testing
-------
I have tested it with current trunk - everything seems to be ok.
Thanks,
Jan Gerrit
Aaron Seigo
2010-06-21 02:40:37 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4414/#review6205
-----------------------------------------------------------



/trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/4414/#comment5799>

the check for context still being relevant should remain, so that the runner can abort this run quickly if the query has changed.


- Aaron
Post by Jan Gerrit Marker
-----------------------------------------------------------
http://reviewboard.kde.org/r/4414/
-----------------------------------------------------------
(Updated 2010-06-20 16:40:04)
Review request for Plasma.
Summary
-------
1. Firefox intern bookmarks (like the "Recently used bookmarks" folder) are not shown
2. Only bookmarks with a non empty name and url are shown
3. The profile written to the config file is updated if it does not exist anymore
Additionally I polished the Firefox regarding method.
I did not commit directly because I was not sure whether it is allowed at this point of the KDE SC development.
Diffs
-----
/trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp 1140335
Diff: http://reviewboard.kde.org/r/4414/diff
Testing
-------
I have tested it with current trunk - everything seems to be ok.
Thanks,
Jan Gerrit
Jan Gerrit Marker
2010-06-22 15:16:46 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4414/
-----------------------------------------------------------

(Updated 2010-06-22 15:16:45.874909)


Review request for Plasma.


Changes
-------

Fixes the issue pointed out by Aaron Seigo.


Summary
-------

Corrects the behavior of the bookmarks runner regarding Firefox:
1. Firefox intern bookmarks (like the "Recently used bookmarks" folder) are not shown
2. Only bookmarks with a non empty name and url are shown
3. The profile written to the config file is updated if it does not exist anymore

Additionally I polished the Firefox regarding method.
I did not commit directly because I was not sure whether it is allowed at this point of the KDE SC development.


Diffs (updated)
-----

/trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp 1141326

Diff: http://reviewboard.kde.org/r/4414/diff


Testing
-------

I have tested it with current trunk - everything seems to be ok.


Thanks,

Jan Gerrit
Aaron Seigo
2010-06-22 16:32:47 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4414/#review6240
-----------------------------------------------------------



/trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp
<http://reviewboard.kde.org/r/4414/#comment5839>

it shouldn't continue the loop when the context is no longer valid (and therefore go through all url's in the bookmarks), it should return.


- Aaron
Post by Jan Gerrit Marker
-----------------------------------------------------------
http://reviewboard.kde.org/r/4414/
-----------------------------------------------------------
(Updated 2010-06-22 15:16:45)
Review request for Plasma.
Summary
-------
1. Firefox intern bookmarks (like the "Recently used bookmarks" folder) are not shown
2. Only bookmarks with a non empty name and url are shown
3. The profile written to the config file is updated if it does not exist anymore
Additionally I polished the Firefox regarding method.
I did not commit directly because I was not sure whether it is allowed at this point of the KDE SC development.
Diffs
-----
/trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp 1141326
Diff: http://reviewboard.kde.org/r/4414/diff
Testing
-------
I have tested it with current trunk - everything seems to be ok.
Thanks,
Jan Gerrit
Jan Gerrit Marker
2010-06-22 16:41:18 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4414/
-----------------------------------------------------------

(Updated 2010-06-22 16:41:18.325281)


Review request for Plasma.


Changes
-------

Correct the last fix about context validation.


Summary
-------

Corrects the behavior of the bookmarks runner regarding Firefox:
1. Firefox intern bookmarks (like the "Recently used bookmarks" folder) are not shown
2. Only bookmarks with a non empty name and url are shown
3. The profile written to the config file is updated if it does not exist anymore

Additionally I polished the Firefox regarding method.
I did not commit directly because I was not sure whether it is allowed at this point of the KDE SC development.


Diffs (updated)
-----

/trunk/KDE/kdebase/workspace/plasma/generic/runners/bookmarks/bookmarksrunner.cpp 1141326

Diff: http://reviewboard.kde.org/r/4414/diff


Testing
-------

I have tested it with current trunk - everything seems to be ok.


Thanks,

Jan Gerrit

Loading...