Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/#review38038 --- Ship it! sorry for taking so long. I applied the patch and everything worked. nice script btw. - Michael Bohlender On Aug. 14, 2013, 7:30 a.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 14, 2013, 7:30 a.m.) Review request for kdelibs, Michael Bohlender and Stephen Kelly. Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/#review38042 --- This review has been submitted with commit 14e2ff5b7359d572cd520fd7da3791230ded169a by Aurélien Gâteau to branch KDE/4.11. - Commit Hook On Aug. 14, 2013, 7:30 a.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 14, 2013, 7:30 a.m.) Review request for kdelibs, Michael Bohlender and Stephen Kelly. Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 17, 2013, 7:45 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs, Michael Bohlender and Stephen Kelly. Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 14, 2013, 7:30 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs, Michael Bohlender and Stephen Kelly. Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
On Aug. 14, 2013, 7:30 a.m., Commit Hook wrote: This review has been submitted with commit 5bab478dd04d85a6017c54bc986bcf0b9e235264 by Michael Bohlender to branch master. What is this about? Was it pushed to a local clone or so? - Stephen --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/#review37747 --- On Aug. 14, 2013, 7:30 a.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 14, 2013, 7:30 a.m.) Review request for kdelibs, Michael Bohlender and Stephen Kelly. Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
On Aug. 9, 2013, 2:59 p.m., Stephen Kelly wrote: Have you tried kdepim applications with this patch too? Many of them use this class. Aurélien Gâteau wrote: I just grepped in kdepim, it seems only kmail-mobile makes use of it. It looks like it's working, but I am not familiar with this application so I am adding mbholender to the reviewer. Michael Bohlender wrote: I am only a GSoC student and I am not familiar with the breadcrumbnavigation part where KLinkItemSelectionModel is used. The patch looks fine to me but I can test it with kmail-mobile if you want me to. I don't know the kdelibs workflow. Could you push a feature branch that I can quickly checkout and build or do I need to apply the patch manually to test it ? Would be awesome if you could test it locally. If you already have a kdelibs build it should be straightforward to apply the patch and build with it (or you can use this script I put together some time ago to apply reviewboard patches: https://gist.github.com/agateau/3637858 ) - Aurélien --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/#review37417 --- On Aug. 14, 2013, 9:30 a.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 14, 2013, 9:30 a.m.) Review request for kdelibs, Michael Bohlender and Stephen Kelly. Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 13, 2013, 5:57 p.m.) Review request for kdelibs and Stephen Kelly. Changes --- Remove empty line. Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs (updated) - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
On Aug. 9, 2013, 2:59 p.m., Stephen Kelly wrote: Have you tried kdepim applications with this patch too? Many of them use this class. I just grepped in kdepim, it seems only kmail-mobile makes use of it. It looks like it's working, but I am not familiar with this application so I am adding mbholender to the reviewer. - Aurélien --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/#review37417 --- On Aug. 13, 2013, 5:57 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 13, 2013, 5:57 p.m.) Review request for kdelibs and Stephen Kelly. Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 13, 2013, 5:59 p.m.) Review request for kdelibs, Michael Bohlender and Stephen Kelly. Changes --- Added mbohlender to the review since kmail-mobile uses KLinkItemSelectionModel Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
On Aug. 9, 2013, 12:59 p.m., Stephen Kelly wrote: Have you tried kdepim applications with this patch too? Many of them use this class. Aurélien Gâteau wrote: I just grepped in kdepim, it seems only kmail-mobile makes use of it. It looks like it's working, but I am not familiar with this application so I am adding mbholender to the reviewer. I am only a GSoC student and I am not familiar with the breadcrumbnavigation part where KLinkItemSelectionModel is used. The patch looks fine to me but I can test it with kmail-mobile if you want me to. I don't know the kdelibs workflow. Could you push a feature branch that I can quickly checkout and build or do I need to apply the patch manually to test it ? - Michael --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/#review37417 --- On Aug. 13, 2013, 3:59 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 13, 2013, 3:59 p.m.) Review request for kdelibs, Michael Bohlender and Stephen Kelly. Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
Re: Review Request 111953: KLinkItemSelectionModel: synchronize currentIndex
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/#review37417 --- Have you tried kdepim applications with this patch too? Many of them use this class. kdeui/itemviews/klinkitemselectionmodel.cpp http://git.reviewboard.kde.org/r/111953/#comment27688 Excess whitespace. - Stephen Kelly On Aug. 8, 2013, 8:15 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111953/ --- (Updated Aug. 8, 2013, 8:15 p.m.) Review request for kdelibs and Stephen Kelly. Description --- In addition to synchronizing selection, make KLinkItemSelectionModel also synchronize the current index. This fixes a bug in Gwenview where image operations are not applied to the correct images when multiple images are selected. This addresses bug 322850. http://bugs.kde.org/show_bug.cgi?id=322850 Diffs - kdeui/itemviews/klinkitemselectionmodel.h 392da46 kdeui/itemviews/klinkitemselectionmodel.cpp ee55d4f kdeui/tests/klinkitemselectionmodeltest.h f3e0fd1 kdeui/tests/klinkitemselectionmodeltest.cpp c3f7132 Diff: http://git.reviewboard.kde.org/r/111953/diff/ Testing --- - Extended unit-tests to test the current index is synchronized. - Verified the bug is fixed in Gwenview. Thanks, Aurélien Gâteau
