DevHeads.net

Review Request 117175: Fix installing new .comic packages from GHNS to appear in the installed packages list in the comic widget.

Review request for KDE Runtime, Albert Astals Cid, Aaron J. Seigo, Ian Monroe, and Lamarque Souza.

Bugs: 306279 and 325028
<a href="http://bugs.kde.org/show_bug.cgi?id=306279" title="http://bugs.kde.org/show_bug.cgi?id=306279">http://bugs.kde.org/show_bug.cgi?id=306279</a>
<a href="http://bugs.kde.org/show_bug.cgi?id=325028" title="http://bugs.kde.org/show_bug.cgi?id=325028">http://bugs.kde.org/show_bug.cgi?id=325028</a>

Repository: kde-runtime

Description
When installing a new .comic provider from GHNS, it doesn't appear in the installed list on the comic widget.
This fixes it.

Diffs
plasma/tools/plasmapkg/main.cpp 61492fe

Diff: <a href="https://git.reviewboard.kde.org/r/117175/diff/" title="https://git.reviewboard.kde.org/r/117175/diff/">https://git.reviewboard.kde.org/r/117175/diff/</a>

Testing
Compile, add new comic widget, install new comic providers.

Thanks,

Andrei Amuraritei

Comments

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 03/30/2014 - 08:28

(Updated March 30, 2014, 12:28 p.m.)

Review request for KDE Runtime, Albert Astals Cid, Aaron J. Seigo, Ian Monroe, and Lamarque Souza.

Changes
Attached a plain diff -u -p orig fixed patch file. Tried updating the diff in the review req. I get the diff is empty message.

Bugs: 306279 and 325028
<a href="http://bugs.kde.org/show_bug.cgi?id=306279" title="http://bugs.kde.org/show_bug.cgi?id=306279">http://bugs.kde.org/show_bug.cgi?id=306279</a>
<a href="http://bugs.kde.org/show_bug.cgi?id=325028" title="http://bugs.kde.org/show_bug.cgi?id=325028">http://bugs.kde.org/show_bug.cgi?id=325028</a>

Repository: kde-runtime

Description
When installing a new .comic provider from GHNS, it doesn't appear in the installed list on the comic widget.
This fixes it.

Diffs
plasma/tools/plasmapkg/main.cpp 61492fe

Diff: <a href="https://git.reviewboard.kde.org/r/117175/diff/" title="https://git.reviewboard.kde.org/r/117175/diff/">https://git.reviewboard.kde.org/r/117175/diff/</a>

Testing
Compile, add new comic widget, install new comic providers.

File Attachments (updated)
patch with diff
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/03/30/313c693a-4c1e-4def-9c8f-4ab949d01874__kde-runtime-diff.patch" title="https://git.reviewboard.kde.org/media/uploaded/files/2014/03/30/313c693a-4c1e-4def-9c8f-4ab949d01874__kde-runtime-diff.patch">https://git.reviewboard.kde.org/media/uploaded/files/2014/03/30/313c693a...</a>

Thanks,

Andrei Amuraritei

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 03/30/2014 - 09:13

(Updated March 30, 2014, 1:13 p.m.)

Review request for KDE Runtime, Albert Astals Cid, Aaron J. Seigo, Ian Monroe, and Lamarque Souza.

Changes
Sorry I'm new to this, just updated hopefully with a correct patch.

Bugs: 306279 and 325028
<a href="http://bugs.kde.org/show_bug.cgi?id=306279" title="http://bugs.kde.org/show_bug.cgi?id=306279">http://bugs.kde.org/show_bug.cgi?id=306279</a>
<a href="http://bugs.kde.org/show_bug.cgi?id=325028" title="http://bugs.kde.org/show_bug.cgi?id=325028">http://bugs.kde.org/show_bug.cgi?id=325028</a>

Repository: kde-runtime

Description
When installing a new .comic provider from GHNS, it doesn't appear in the installed list on the comic widget.
This fixes it.

Diffs (updated)
plasma/tools/plasmapkg/main.cpp 61492fe

Diff: <a href="https://git.reviewboard.kde.org/r/117175/diff/" title="https://git.reviewboard.kde.org/r/117175/diff/">https://git.reviewboard.kde.org/r/117175/diff/</a>

Testing
Compile, add new comic widget, install new comic providers.

Thanks,

Andrei Amuraritei

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 04/06/2014 - 02:57

(Updated April 6, 2014, 6:57 a.m.)

Review request for KDE Runtime, Albert Astals Cid, Aaron J. Seigo, Ian Monroe, and Lamarque Souza.

Changes
More specific fix, and comments added. To work this depends on <a href="https://git.reviewboard.kde.org/r/117174/" title="https://git.reviewboard.kde.org/r/117174/">https://git.reviewboard.kde.org/r/117174/</a>

Bugs: 306279 and 325028
<a href="http://bugs.kde.org/show_bug.cgi?id=306279" title="http://bugs.kde.org/show_bug.cgi?id=306279">http://bugs.kde.org/show_bug.cgi?id=306279</a>
<a href="http://bugs.kde.org/show_bug.cgi?id=325028" title="http://bugs.kde.org/show_bug.cgi?id=325028">http://bugs.kde.org/show_bug.cgi?id=325028</a>

Repository: kde-runtime

Description
When installing a new .comic provider from GHNS, it doesn't appear in the installed list on the comic widget.
This fixes it.

Diffs (updated)
plasma/tools/plasmapkg/main.cpp 61492fe

Diff: <a href="https://git.reviewboard.kde.org/r/117175/diff/" title="https://git.reviewboard.kde.org/r/117175/diff/">https://git.reviewboard.kde.org/r/117175/diff/</a>

Testing
Compile, add new comic widget, install new comic providers.

Thanks,

Andrei Amuraritei

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 04/23/2014 - 19:05

(Updated April 23, 2014, 11:05 p.m.)

Review request for KDE Runtime, Aaron J. Seigo, Ian Monroe, and Lamarque Souza.

Bugs: 306279 and 325028
<a href="http://bugs.kde.org/show_bug.cgi?id=306279" title="http://bugs.kde.org/show_bug.cgi?id=306279">http://bugs.kde.org/show_bug.cgi?id=306279</a>
<a href="http://bugs.kde.org/show_bug.cgi?id=325028" title="http://bugs.kde.org/show_bug.cgi?id=325028">http://bugs.kde.org/show_bug.cgi?id=325028</a>

Repository: kde-runtime

Description
When installing a new .comic provider from GHNS, it doesn't appear in the installed list on the comic widget.
This fixes it.

Diffs
plasma/tools/plasmapkg/main.cpp 61492fe

Diff: <a href="https://git.reviewboard.kde.org/r/117175/diff/" title="https://git.reviewboard.kde.org/r/117175/diff/">https://git.reviewboard.kde.org/r/117175/diff/</a>

Testing
Compile, add new comic widget, install new comic providers.

Thanks,

Andrei Amuraritei

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 08/18/2014 - 16:21

(Updated Aug. 18, 2014, 11:21 p.m.)

Review request for KDE Runtime, Aaron J. Seigo, Ian Monroe, and Lamarque Souza.

Bugs: 306279 and 325028
<a href="http://bugs.kde.org/show_bug.cgi?id=306279" title="http://bugs.kde.org/show_bug.cgi?id=306279">http://bugs.kde.org/show_bug.cgi?id=306279</a>
<a href="http://bugs.kde.org/show_bug.cgi?id=325028" title="http://bugs.kde.org/show_bug.cgi?id=325028">http://bugs.kde.org/show_bug.cgi?id=325028</a>

Repository: kde-runtime

Description
When installing a new .comic provider from GHNS, it doesn't appear in the installed list on the comic widget.
This fixes it.

Diffs (updated)
plasma/tools/plasmapkg/main.cpp 61492fe

Diff: <a href="https://git.reviewboard.kde.org/r/117175/diff/" title="https://git.reviewboard.kde.org/r/117175/diff/">https://git.reviewboard.kde.org/r/117175/diff/</a>

Testing
Compile, add new comic widget, install new comic providers.

Thanks,

Andrei Amuraritei

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 08/20/2014 - 22:10

(Updated Aug. 21, 2014, 5:10 a.m.)

Status
This change has been marked as submitted.

Review request for KDE Runtime, Aaron J. Seigo, Ian Monroe, and Lamarque Souza.

Bugs: 306279 and 325028
<a href="http://bugs.kde.org/show_bug.cgi?id=306279" title="http://bugs.kde.org/show_bug.cgi?id=306279">http://bugs.kde.org/show_bug.cgi?id=306279</a>
<a href="http://bugs.kde.org/show_bug.cgi?id=325028" title="http://bugs.kde.org/show_bug.cgi?id=325028">http://bugs.kde.org/show_bug.cgi?id=325028</a>

Repository: kde-runtime

Description
When installing a new .comic provider from GHNS, it doesn't appear in the installed list on the comic widget.
This fixes it.

Diffs
plasma/tools/plasmapkg/main.cpp 61492fe

Diff: <a href="https://git.reviewboard.kde.org/r/117175/diff/" title="https://git.reviewboard.kde.org/r/117175/diff/">https://git.reviewboard.kde.org/r/117175/diff/</a>

Testing
Compile, add new comic widget, install new comic providers.

Thanks,

Andrei Amuraritei

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 08/18/2014 - 16:21

Ship it!

Ship It!

- Andrei Amuraritei

On Aug. 18, 2014, 11:21 p.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Marco Martin at 08/15/2014 - 06:40

Ship it!

the explicit listing of themes is ok, puts it in line with plasmapkg2.
dead code should be removed before shipping

plasma/tools/plasmapkg/main.cpp
<https://git.reviewboard.kde.org/r/117175/#comment45142>

never comment dead code, if it should be removed, should just be deleted

- Marco Martin

On Apr. 23, 2014, 11:05 p.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Rodrigo Belem at 08/08/2014 - 16:30

The patch <a href="https://git.reviewboard.kde.org/r/117174/" title="https://git.reviewboard.kde.org/r/117174/">https://git.reviewboard.kde.org/r/117174/</a> got a ship it.

- Rodrigo Belem

On April 23, 2014, 11:05 p.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 03/30/2014 - 08:35

File Attachment: patch with diff - kde-runtime-diff.patch
<https://git.reviewboard.kde.org//r/117175/#fcomment205>
The original patch was made with git diff.

- Andrei Amuraritei

On March 30, 2014, 12:28 p.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Lamarque V. Souza at 03/30/2014 - 07:28

By the way, there must be a "delete installer;" before the mentioned "return 1" line, otherwise we will have a memory leak.

- Lamarque Souza

On March 30, 2014, 3:47 a.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 03/30/2014 - 08:34

Also the installer is deleted finally at the end when successful.

- Andrei

On March 30, 2014, 12:28 p.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Lamarque V. Souza at 03/30/2014 - 07:23

Hi, there is an error when trying to show the patch on reviewboard. Can you provide the correct patch?

I looked into the raw patch and I think the "return 1" line that you commented should be kept when the action is not upgrade.

- Lamarque Souza

On March 30, 2014, 3:47 a.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 03/30/2014 - 17:03

Actually it would be package = /home/1.comic The whole path. On further inspecting the flow, I find it weird that check for package.isEmpty is at the bottom when it should have been after the seq you mentioned.
Also thanks for you time in reviewing this issue.

- Andrei

On March 30, 2014, 1:13 p.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 03/30/2014 - 16:53

package = args->getOption("remove"); This line retrieves the package from command line args after the 'remove' option.
For example plasmapkg -t comic -r /home/1.comic. package would be equal to 1.comic in this case. Action would be 'remove'.

- Andrei

On March 30, 2014, 1:13 p.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Lamarque V. Souza at 03/30/2014 - 14:40

The lines below are at the beginning of function the code in question resides:

if (args->isSet("remove")) {
package = args->getOption("remove");
}

So package will not be empty when using 'remove' action, then runKbuildsycoca() will get called because the check package.isEmpty() will return false. Am I correct?

- Lamarque

On March 30, 2014, 1:13 p.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 03/30/2014 - 14:14

runKbuildsycoca() doesn't get called unless there's a package specified with an action to execute. The check package.isEmpty() asures that.

- Andrei

On March 30, 2014, 1:13 p.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Lamarque V. Souza at 03/30/2014 - 10:13

I agree that the code should not return when action is 'upgrade'. When action is 'remove' and nothing is actually removed (because there is no package to remove in this case) runKbuildsycoca() will be called unnecessarily some lines later, that is what I am trying to avoid. As long as calling runKbuildsycoca() here is ok for the other reviewers I am ok with your patch.

- Lamarque

On March 30, 2014, 1:13 p.m., Andrei Amuraritei wrote:

Re: Review Request 117175: Fix installing new .comic packages fr

By Deiu at 03/30/2014 - 08:34

The case tested there are actions 'remove' or 'upgrade'. Since that gets called for a remove action there is a delete installer in case of failure to remove the package as in line 409 and 410.
For upgrade action if the "return 1" line is left then there is no install or upgrade action executed further down with the package.

- Andrei

On March 30, 2014, 12:28 p.m., Andrei Amuraritei wrote: