Re: Review Request 102391: Don't hang when determining MIME type of corrupted files
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102391/ --- (Updated Oct. 27, 2013, 7:02 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Bugs: 280446 http://bugs.kde.org/show_bug.cgi?id=280446 Repository: kdelibs Description --- If KMimeTypeRepository::findFromContent() tries to determine MIME from a file that cannot be read, such as on a corrupted optical disc, a read attempt is made in KMimeMagicMatch::match() for every available rule, resulting in UI hangs (e.g. file dialogs, dolphin) for tens of minutes (see https://bugs.kde.org/show_bug.cgi?id=280446 for more details). I've submitted this patch here on behalf of Miroslav ?os, who has submitted the bug-report and also has written the patch. Diffs - kdecore/services/kmimetype.cpp 955bf62 kdecore/services/kmimetyperepository.cpp 6ff3d16 Diff: http://git.reviewboard.kde.org/r/102391/diff/ Testing --- Thanks, Peter Penz
Re: Review Request: [dolphin] Fix the warning generated by desktop-file-validate for dolphin.desktop
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105288/#review14833 --- Ship it! Thanks, looks fine! (btw: from my point of view no review-request would have been necessary for this fix, but of course it is exemplary doing it this way :-)) - Peter Penz On June 18, 2012, 1:47 a.m., Jekyll Wu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105288/ --- (Updated June 18, 2012, 1:47 a.m.) Review request for KDE Base Apps and Peter Penz. Description --- desktop-file-validate dolphin.dekstop generates this: dolphin.desktop: error: (will be fatal in the future): value FileManager in key Categories in group Desktop Entry requires another category to be present among the following categories: System;FileTools FDO menu specification[1] suggests System;FileTools should be used together with the FileManager category. The patch simply adds the missing FileTools [1] http://standards.freedesktop.org/menu-spec/latest/apa.html Diffs - dolphin/src/dolphin.desktop cd9da17 Diff: http://git.reviewboard.kde.org/r/105288/diff/ Testing --- Thanks, Jekyll Wu
Re: KDE SC 4.8.4 important problems
On 06/10/2012 11:20 AM, Aaron J. Seigo wrote: On Sunday, June 10, 2012 03:23:04 José Manuel Santamaría Lema wrote: #1 dolphin: #2 gwenview #6 kontact executing various components: calendar, to-do list, journal #7 kmail links these are all the same crash, or at least related to each other. it is crashing in KServiceTypeTrader::defaultOffers or KMimeTypeTrader::query apparently at times in KSycocaDict::find_string. The issue has been tracked at https://bugs.kde.org/show_bug.cgi?id=268064 - updating Soprano to the latest master resolves the crash. But I don't know more about the root-cause of this. Probably a Nepomuk-related update missed a proper versioning-check of Soprano? #3 kontact executing the kaddressbook component #4 kmail executed outside kontact these need to be sent to the kdepim team as they are crashes in kdepim components (address book library, kmail editor, ...)
Re: Review Request: show video previews according to file content instead of mimetype string
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104988/#review13988 --- Thanks a lot for the patch, looks good! I've just tested it: It works fine and I've pushed it to master. I initially was a little bit confused about the semantics of the hasVideoChanged()-signal and wanted to adjust the name of the signal. But I've seen you've taken the same name as Phonon uses in the MediaObject and probably we should stay consistent here. So I've only added a small comment to the signal... - Peter Penz On May 19, 2012, 6:54 a.m., Hui Ni wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104988/ --- (Updated May 19, 2012, 6:54 a.m.) Review request for Dolphin, KDE Base Apps and Peter Penz. Description --- This patch makes dolphin information panel to show a video widget depending on the video content instead of mimetype string. There are container formats which can be either audios or videos. Besides, the rmvb video files have mimetype of application/vnd.rn-realmedia, and these files can be recognized as videos correctly now. Diffs - dolphin/src/panels/information/informationpanelcontent.h 1d964f5 dolphin/src/panels/information/informationpanelcontent.cpp 4a96bd1 dolphin/src/panels/information/phononwidget.h 1e1ea37 dolphin/src/panels/information/phononwidget.cpp 5f0c111 Diff: http://git.reviewboard.kde.org/r/104988/diff/ Testing --- with no more regressions. Thanks, Hui Ni
Re: Review Request: Ensure authentication data is cached properly in Dolphin
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104614/#review12509 --- Thanks for the patch, I've not been aware that this caching-issue can be solved like this :-) I guess this patch has not been applied to the latest master: DolphinView does not have a dir-lister anymore (it has been moved to KFileItemModel). Anyhow: Please just let me rebase the patch myself so that I can catch all cases - I'll push the patch to master during the next days. - Peter Penz On April 16, 2012, 1:26 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104614/ --- (Updated April 16, 2012, 1:26 p.m.) Review request for KDE Base Apps and Peter Penz. Description --- The attached patch sets the main window on the main directory lister in DolphinView and KIO jobs in DolphinMainWindow to ensure that login data for remote protocols such as sftp, ftp are cached properly for the duration of the application. Otherwise, the end user is going to end up being unnecessarily re-prompted to enter password login information. Please note that this patch does not address every use of KIO or KDirLister in Dolphin. There are other places in Dolphin's code where directory listers and/or jobs are used without setting the main window. Diffs - dolphin/src/dolphinmainwindow.cpp 947db77 dolphin/src/views/dolphinview.cpp 78fd56d Diff: http://git.reviewboard.kde.org/r/104614/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Ensure authentication data is cached properly in Dolphin
On April 16, 2012, 2:03 p.m., Peter Penz wrote: Thanks for the patch, I've not been aware that this caching-issue can be solved like this :-) I guess this patch has not been applied to the latest master: DolphinView does not have a dir-lister anymore (it has been moved to KFileItemModel). Anyhow: Please just let me rebase the patch myself so that I can catch all cases - I'll push the patch to master during the next days. Dawit Alemayehu wrote: Ahh indeed. I forgot to update kde-baseapps before creating the patch. :( As far as not being aware of resolving password caching issues this was, I guess that is my failure for not informing people about such things. I will have to start blogging about such issues in the future. Anyhow, I will let you deal with it. BTW, on a related not, is it a good idea to call KDirLister from a KIO slave as it is currently being done in src/search/filenamesearchprotocol.cpp ? After all KDirLister itself relies on KIO itself. Why not simply use QDir::entryList or QDir::entryInfoList ? Since ioslaves are run in a different process, the fact that those two functions are blocking functions won't much matter. Regarding the KDirLister using in the filenamesearchprotocol: The reason that I did it this way was that it should be possible to also do a search e.g. on a ftp-repository. Are there technical reasons or risks when doing it this way? I initially was unsure whether this can work at all, but it turned out to run very smooth. - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104614/#review12509 --- On April 16, 2012, 1:26 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104614/ --- (Updated April 16, 2012, 1:26 p.m.) Review request for KDE Base Apps and Peter Penz. Description --- The attached patch sets the main window on the main directory lister in DolphinView and KIO jobs in DolphinMainWindow to ensure that login data for remote protocols such as sftp, ftp are cached properly for the duration of the application. Otherwise, the end user is going to end up being unnecessarily re-prompted to enter password login information. Please note that this patch does not address every use of KIO or KDirLister in Dolphin. There are other places in Dolphin's code where directory listers and/or jobs are used without setting the main window. Diffs - dolphin/src/dolphinmainwindow.cpp 947db77 dolphin/src/views/dolphinview.cpp 78fd56d Diff: http://git.reviewboard.kde.org/r/104614/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Fix signal/slot connections in kcmdolphinviewmodes
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104033/#review10769 --- Ship it! Thanks for the patch, looks fine! - Peter Penz On Feb. 20, 2012, 6:15 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104033/ --- (Updated Feb. 20, 2012, 6:15 p.m.) Review request for KDE Base Apps and Peter Penz. Description --- The attached patch fixes a QObject no such slot warning messages in Dolphin's viewmode kcm. Diffs - dolphin/src/settings/kcm/kcmdolphinviewmodes.h 4ec29db dolphin/src/settings/kcm/kcmdolphinviewmodes.cpp 5441b78 Diff: http://git.reviewboard.kde.org/r/104033/diff/ Testing --- Thanks, Dawit Alemayehu
Re: Review Request: Show the correct remote charset encoding in Konqueror's and Dolphin's Set Remote Encoding menu
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103730/#review9929 --- Ship it! Thanks for the patch, looks fine! - Peter Penz On Jan. 18, 2012, 7:03 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103730/ --- (Updated Jan. 18, 2012, 7:03 p.m.) Review request for KDE Base Apps and Peter Penz. Description --- The attached patch fixes a logic error in the code that determines which remote encoding should be checked when the Show Remote Encoding menu is shown. The logic flaw only affects when the user chooses an encoding which has similar types, e.g. ISO-8859-1*. This addresses bug 186289. http://bugs.kde.org/show_bug.cgi?id=186289 Diffs - dolphin/src/views/dolphinremoteencoding.cpp 8644f5c Diff: http://git.reviewboard.kde.org/r/103730/diff/diff Testing --- 1.) Connect to a remote server. 2.) Change the remote charset encoding to Western European ( ISO-8859-1 ). 3.) Go back to the remote encoding and check what is selected. Thanks, Dawit Alemayehu
Re: Review Request: Fix capacity text in places panel (and possibly unwanted spin-up behavior)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103682/#review9772 --- I'm not the maintainer of the code, but the patch looks good. Definitely a go from a Dolphin point of view to get this in before 4.8.0 got tagged. Thanks a lot for this patch - I stumbled above those issues myself already but did not have the time yet to check KFilePlacesView... - Peter Penz On Jan. 12, 2012, 4:42 p.m., Christoph Feck wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103682/ --- (Updated Jan. 12, 2012, 4:42 p.m.) Review request for Dolphin, kdelibs and Solid. Description --- I only tried to fix the double/blurry text (bug 248062), but stumbled upon a possible reason for the unwanted spin-up behavior reported in the other bugs. According to the code, the KFreeDiskSpaceInfo object is queried even if we do not display the capacity bar. The attached patch should fix it: - the text is not faded at two positions, but moved between those two positions - the KFreeDiskSpaceInfo object is only created if we indeed want to show the capacity bar I tried to separate the patches for those two issues, but it was not possible. Please review. This addresses bugs 184449, 248062, 261552, 264487, and 268103. http://bugs.kde.org/show_bug.cgi?id=184449 http://bugs.kde.org/show_bug.cgi?id=248062 http://bugs.kde.org/show_bug.cgi?id=261552 http://bugs.kde.org/show_bug.cgi?id=264487 http://bugs.kde.org/show_bug.cgi?id=268103 Diffs - kfile/kfileplacesview.cpp 6a343b3 Diff: http://git.reviewboard.kde.org/r/103682/diff/diff Testing --- Thanks, Christoph Feck
Re: Review Request: Fix stray ampersand in KUrlNavigatorProtocolCombo
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103040/#review7928 --- Ship it! Thanks Christoph, looks perfect! - Peter Penz On Nov. 3, 2011, 7:27 p.m., Christoph Feck wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103040/ --- (Updated Nov. 3, 2011, 7:27 p.m.) Review request for Dolphin and kdelibs. Description --- - fix sizeHint to not include the accelerator - respect styles' hint for showing mnemonics - respect widgets' enabled() state when drawing the label This addresses bug 285163. http://bugs.kde.org/show_bug.cgi?id=285163 Diffs - kfile/kurlnavigatorprotocolcombo.cpp 1a08a4c Diff: http://git.reviewboard.kde.org/r/103040/diff/diff Testing --- Thanks, Christoph Feck
Re: Review Request: kfileplaceeditdialog lineedit too small
On Oct. 5, 2011, 11:30 a.m., David Faure wrote: Why the setMaxLength?? What if one wants to type in a long URL? Also, I can't reproduce the bug here (kde-4.7), but maybe only because the big icon button makes the dialog quite large? Greg T wrote: indeed, the setmaxLength was stupid it's quite small on 4.6.5 http://wstaw.org/m/2011/10/05/plasma-desktopF27745.jpg David Faure wrote: Ah I see, it's the german translation which makes the label longer (Choose an icon - Wählen Sie ein Symbol aus), leaving less room for the lineedits. Patch 3 looks good, although I can't help but think there's a bug if a set*Width() call takes a height() as value :-) But I'll trust Christoph on this one. Patch 3 looks good, although I can't help but think there's a bug if a set*Width() call takes a height() as value :-) But I'll trust Christoph on this one. Using QFontMetrics::averageCharWidth() might be an alternative in comparison to use the height as reference for calculating the width (but probably this has some drawbacks I'm not aware of). - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102751/#review7117 --- On Oct. 5, 2011, 6:22 p.m., Greg T wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102751/ --- (Updated Oct. 5, 2011, 6:22 p.m.) Review request for Dolphin and kdelibs. Description --- Hi, this patch sets a minimum size for the widget. This addresses bug 266435. http://bugs.kde.org/show_bug.cgi?id=266435 Diffs - kfile/kfileplaceeditdialog.cpp d798b4d Diff: http://git.reviewboard.kde.org/r/102751/diff/diff Testing --- it's working Thanks, Greg T
Re: Review Request: W7 Tab thumbnails in dolphin.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102758/#review7010 --- I always recommend to get in contact with the maintainers of an application _before_ investigating so much work into a new feature. In this case I'm very sorry to say that this cannot get pushed because of the following reasons: - I'm unable to maintain this code as I don't do any Dolphin development on Windows (and cannot do it because of having limited time) - I'd like to keep platform dependent code in Dolphin as minimal as possible - For the 4.9 release of the KDE applications (= Dolphin 2.1) a long overdue cleanup of DolphinMainWindow will be done (separated code for tabs etc) and I won't be able to refactor this platform specific code :-( - Peter Penz On Oct. 3, 2011, 1:25 a.m., Andrius da Costa Ribas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102758/ --- (Updated Oct. 3, 2011, 1:25 a.m.) Review request for KDE Base Apps, KDE Accessibility, kdewin, Patrick Spendrin, and Peter Penz. Description --- Add Windows 7 tab thumbnails feature to dolphin. Mostly based on the example from http://nicug.blogspot.com/2011/03/windows-7-taskbar-extensions-in-qt-tab.html. An icon representation is used instead of actual thumbnails ( please agree that those microscopic previews are not useful at all ;] ). Changing an icon when url changes is also easier than checking all the time whether something inside the window has been changed. Using icons is a lot more KDE-ish and therefore more beautiful and user-friendly than the default Windows behavior ;). win7utils.h and win7utils.cpp are from https://github.com/xfreebird/blogstuff/tree/master/qt/thumbnailtabs_example1 with few adaptations. Diffs - dolphin/src/CMakeLists.txt 93225c5 dolphin/src/dolphinapplication.h 69d07c3 dolphin/src/dolphinapplication.cpp 0dc9c96 dolphin/src/dolphinmainwindow.h 9fb83bf dolphin/src/dolphinmainwindow.cpp 6ca6e59 dolphin/src/platform/win7utils.h PRE-CREATION dolphin/src/platform/win7utils.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/102758/diff/diff Testing --- Tested using MSVC 2010 32 bit, in a Windows 7 64 bit machine. May need testing: - Compiling under mingw-w32 and mingw-w64 - Using Windows Vista or below to ensure there are no side-efects on a box without this feature. - Using Windows 8 (I don't know much about its bugs^H^H^H^Hfeatures ;] ) Known problems: - There is no way to know if KTabBar got a tab reordered, so the thumbnails won't be reordered, but their reference is still correct - Undefined behavior when dolphin gets unresponsive [e.g.: because of a defective kioslave], most of the code assume dolphin is okay [e.g.: QPixmap::grabWidget won't work in a frozen window]. Screenshots --- Tabs! http://git.reviewboard.kde.org/r/102758/s/281/ More Tabs! http://git.reviewboard.kde.org/r/102758/s/282/ Too many tabs! http://git.reviewboard.kde.org/r/102758/s/283/ Thanks, Andrius da Costa Ribas
Re: Review Request: W7 Tab thumbnails in dolphin.
On Oct. 3, 2011, 7:28 a.m., Peter Penz wrote: I always recommend to get in contact with the maintainers of an application _before_ investigating so much work into a new feature. In this case I'm very sorry to say that this cannot get pushed because of the following reasons: - I'm unable to maintain this code as I don't do any Dolphin development on Windows (and cannot do it because of having limited time) - I'd like to keep platform dependent code in Dolphin as minimal as possible - For the 4.9 release of the KDE applications (= Dolphin 2.1) a long overdue cleanup of DolphinMainWindow will be done (separated code for tabs etc) and I won't be able to refactor this platform specific code :-( Patrick Spendrin wrote: I think that we would also maintain this code part as we already do for other parts in KDE, so you normally shouldn't need to work on that. It would be nice if you could let us participate in the refactoring process for 4.9 so that we can adapt this patch early according to your wishes. Sure, I'll drop you a note as soon as I'm cleaning up the mainwindow-code for 4.9. It sounds fine that you would be willing to maintain this code - I'm just generally wondering: Do you plan to add such window-specific code to each KDE application providing tabs? I'm still not really happy with having that much platform specific code inside the application... Well, probably I'll change my opinion but I would be interested how other application developers see this :-) - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102758/#review7010 --- On Oct. 3, 2011, 1:25 a.m., Andrius da Costa Ribas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102758/ --- (Updated Oct. 3, 2011, 1:25 a.m.) Review request for KDE Base Apps, KDE Accessibility, kdewin, Patrick Spendrin, and Peter Penz. Description --- Add Windows 7 tab thumbnails feature to dolphin. Mostly based on the example from http://nicug.blogspot.com/2011/03/windows-7-taskbar-extensions-in-qt-tab.html. An icon representation is used instead of actual thumbnails ( please agree that those microscopic previews are not useful at all ;] ). Changing an icon when url changes is also easier than checking all the time whether something inside the window has been changed. Using icons is a lot more KDE-ish and therefore more beautiful and user-friendly than the default Windows behavior ;). win7utils.h and win7utils.cpp are from https://github.com/xfreebird/blogstuff/tree/master/qt/thumbnailtabs_example1 with few adaptations. Diffs - dolphin/src/CMakeLists.txt 93225c5 dolphin/src/dolphinapplication.h 69d07c3 dolphin/src/dolphinapplication.cpp 0dc9c96 dolphin/src/dolphinmainwindow.h 9fb83bf dolphin/src/dolphinmainwindow.cpp 6ca6e59 dolphin/src/platform/win7utils.h PRE-CREATION dolphin/src/platform/win7utils.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/102758/diff/diff Testing --- Tested using MSVC 2010 32 bit, in a Windows 7 64 bit machine. May need testing: - Compiling under mingw-w32 and mingw-w64 - Using Windows Vista or below to ensure there are no side-efects on a box without this feature. - Using Windows 8 (I don't know much about its bugs^H^H^H^Hfeatures ;] ) Known problems: - There is no way to know if KTabBar got a tab reordered, so the thumbnails won't be reordered, but their reference is still correct - Undefined behavior when dolphin gets unresponsive [e.g.: because of a defective kioslave], most of the code assume dolphin is okay [e.g.: QPixmap::grabWidget won't work in a frozen window]. Screenshots --- Tabs! http://git.reviewboard.kde.org/r/102758/s/281/ More Tabs! http://git.reviewboard.kde.org/r/102758/s/282/ Too many tabs! http://git.reviewboard.kde.org/r/102758/s/283/ Thanks, Andrius da Costa Ribas
Re: Review Request: KVersionControlPlugin2 interface to implement add some features not available in current interface.
On Sept. 6, 2011, 4:28 p.m., Sebastian Doerner wrote: Looks good to me. Peter, are you fine with this? The plugin itself will follow next. Peter Penz wrote: @Frank: I'm fine with the interface extensions! @Vishesh: Thanks for the patch, it looks fine. Please give me a little bit time to get Dolphin 2 into a state where it shows the version plugin states again, I plan to be finished during the next 10 days with this (~ 16. September). I'd like to take the chance when having a KVersionControlPlugin2 interface to also fix some const-errors of the previous interface. But to test this I need first to get back the version control plugins functionality in Dolphin 2 :-) Vishesh Yadav wrote: Ok. So I should put this patch on hold atm, right? Yes, please. I'll contact you as soon as the patch can be merged. - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102541/#review6305 --- On Sept. 6, 2011, 3:51 p.m., Vishesh Yadav wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102541/ --- (Updated Sept. 6, 2011, 3:51 p.m.) Review request for Dolphin, KDE Base Apps, Peter Penz, and Sebastian Doerner. Summary --- Added KVersionControlPlugin2 interface to let version control plugins be able to show context menu anywhere not just in repositories. Will be useful to implement commands like Clone(in Git, Hg) or Checkout(in SVN). Part of GSoC project Mercurial Plugin for Dolphin http://goo.gl/6B2ly Not much changes. Just added one function right now. Diffs - lib/konq/CMakeLists.txt 651beff lib/konq/kversioncontrolplugin.h e6cb2b4 lib/konq/kversioncontrolplugin2.h PRE-CREATION lib/konq/kversioncontrolplugin2.cpp PRE-CREATION Diff: http://git.reviewboard.kde.org/r/102541/diff Testing --- Yes. With my Mercurial plugin and modified Dolphin 1.7 source code, whose patch I havent posted as Dolphin 2 is now coming up. Thanks, Vishesh
Re: Review Request: Dolphin zoom with CTRL+MouseWheel
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102490/#review6167 --- Ship it! Thanks, this is a good idea :-) I could not try the patch myself yet but it looks fine. dolphin/src/kitemviews/kitemlistcontainer.cpp http://git.reviewboard.kde.org/r/102490/#comment5433 Please move the 'if' above the line: KItemList* view = ... As 'view' is not used within the new 'if' - Peter On Aug. 30, 2011, 11:08 a.m., Vishesh Yadav wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102490/ --- (Updated Aug. 30, 2011, 11:08 a.m.) Review request for KDE Base Apps and Peter Penz. Summary --- Zoom DolphinView when mouse wheel is scrolled with CRTL button pressed. Diffs - dolphin/src/kitemviews/kitemlistcontainer.cpp 0d2637d dolphin/src/views/dolphinview.h 7c81ea8 dolphin/src/views/dolphinview.cpp 0991401 Diff: http://git.reviewboard.kde.org/r/102490/diff Testing --- yes Thanks, Vishesh
Re: Review Request: Dolphin zoom with CTRL+MouseWheel
On Aug. 30, 2011, 4:55 p.m., Dominik Haumann wrote: dolphin/src/views/dolphinview.cpp, line 691 http://git.reviewboard.kde.org/r/102490/diff/1/?file=33274#file33274line691 Just a maybe: if event-delta() is 8, numDegrees rounds to 0 due to the int conversion. Same for numSteps. I'm not sure whether this can happen, but I remember a similar issue somewhere else (was it in Kate?)... Thanks for the hint, I've added this to my TODO-list - hope I find some time to check this before the 2.0 release of Dolphin (still there are so many things that must be fixed first ;-)) - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102490/#review6174 --- On Aug. 30, 2011, 11:08 a.m., Vishesh Yadav wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102490/ --- (Updated Aug. 30, 2011, 11:08 a.m.) Review request for KDE Base Apps and Peter Penz. Summary --- Zoom DolphinView when mouse wheel is scrolled with CRTL button pressed. Diffs - dolphin/src/kitemviews/kitemlistcontainer.cpp 0d2637d dolphin/src/views/dolphinview.h 7c81ea8 dolphin/src/views/dolphinview.cpp 0991401 Diff: http://git.reviewboard.kde.org/r/102490/diff Testing --- yes Thanks, Vishesh
Re: Review Request: Allow opening files and directories by pressing 'Enter' or 'Return'
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102450/#review6128
---
Ship it!
Thanks for the update! Looks good and is exactly like the proposal you, Frank
and I discussed per e-mail. As usual I've added a punch of my nitpicking stuff
;-) Please push it to master after fixing, you don't need to add another diff
here.
dolphin/src/views/dolphinview.cpp
http://git.reviewboard.kde.org/r/102450/#comment5407
1. const is missing: const QSetint...
2. I'd suggest to use 'selection' or 'selectedItems' instead of 'itemsSet'
for consistency with the other code.
dolphin/src/views/dolphinview.cpp
http://git.reviewboard.kde.org/r/102450/#comment5408
Remove: It is only used in the else-path
dolphin/src/views/dolphinview.cpp
http://git.reviewboard.kde.org/r/102450/#comment5409
Change to:
if (itemsSet.isEmpty()) {
return;
}
dolphin/src/views/dolphinview.cpp
http://git.reviewboard.kde.org/r/102450/#comment5410
Coding style - braces:
if (itemsSet.count() == 1) {
dolphin/src/views/dolphinview.cpp
http://git.reviewboard.kde.org/r/102450/#comment5411
Replace 'const int i' by just 'int i'. The const-reference in foreach is
fine for classes but unnecessary (and even more expensive) for scalar types
like int.
dolphin/src/views/dolphinview.cpp
http://git.reviewboard.kde.org/r/102450/#comment5412
As the KFileItem declaration has been remove above, use:
const KFileItem fileItem = ...;
dolphin/src/views/dolphinview.cpp
http://git.reviewboard.kde.org/r/102450/#comment5413
Coding style, please use:
} else {
dolphin/src/views/dolphinview.cpp
http://git.reviewboard.kde.org/r/102450/#comment5414
const KFileItem item = ...
dolphin/src/views/dolphinview.cpp
http://git.reviewboard.kde.org/r/102450/#comment5415
1. const KFileItem item = ...;
2. I'd suggest to move it down right before it is needed before 'emit
requestContextMenu'
- Peter
On Aug. 29, 2011, 9:14 a.m., Tirtha Chatterjee wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102450/
---
(Updated Aug. 29, 2011, 9:14 a.m.)
Review request for KDE Base Apps and Peter Penz.
Summary
---
Allow opening files and directories by pressing 'Enter' key. In case multiple
files are selected when enter is pressed, all of them are opened. In case of
multiple directories, the first directory gets opened in the current tab,
while the other directories open in new tabs.
Diffs
-
dolphin/src/kitemviews/kitemlistcontroller.h 04d4985
dolphin/src/kitemviews/kitemlistcontroller.cpp 207535c
dolphin/src/views/dolphinview.h 437f12f
dolphin/src/views/dolphinview.cpp 959e4da
Diff: http://git.reviewboard.kde.org/r/102450/diff
Testing
---
Yes, tested and working.
Thanks,
Tirtha
Re: Review Request: Allow opening files and directories by pressing 'Enter' or 'Return'
On Aug. 29, 2011, 9:40 a.m., Peter Penz wrote: Thanks for the update! Looks good and is exactly like the proposal you, Frank and I discussed per e-mail. As usual I've added a punch of my nitpicking stuff ;-) Please push it to master after fixing, you don't need to add another diff here. Tirtha Chatterjee wrote: I have used itemActivated signal here since itemTriggered is already used by DolphinView. Then I noticed that in my previous keyboard-searching patch, we use the name 'activation' to mean selection. So is it okay to use activation here? QAbstractItemView also uses the signal 'activated()' for the same meaning as we have, so I'd say using itemActivated() instead of itemTriggered() is fine. Would like to hear Frank's opinion about this, but I'd say please push the patch, we can change this afterwards if we change our mind (currently I'd also tend to rename the DolphinView signal to 'itemActivated' for consistency). - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102450/#review6128 --- On Aug. 29, 2011, 9:14 a.m., Tirtha Chatterjee wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102450/ --- (Updated Aug. 29, 2011, 9:14 a.m.) Review request for KDE Base Apps and Peter Penz. Summary --- Allow opening files and directories by pressing 'Enter' key. In case multiple files are selected when enter is pressed, all of them are opened. In case of multiple directories, the first directory gets opened in the current tab, while the other directories open in new tabs. Diffs - dolphin/src/kitemviews/kitemlistcontroller.h 04d4985 dolphin/src/kitemviews/kitemlistcontroller.cpp 207535c dolphin/src/views/dolphinview.h 437f12f dolphin/src/views/dolphinview.cpp 959e4da Diff: http://git.reviewboard.kde.org/r/102450/diff Testing --- Yes, tested and working. Thanks, Tirtha
Re: Review Request: Make Dolphin honour the KGlobalSettings::singleClick option.
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102447/#review6042
---
dolphin/src/kitemviews/kitemlistcontroller.cpp
http://git.reviewboard.kde.org/r/102447/#comment5320
Should be:
bool emitItemClicked = KGlobalSettings::singleClick() ||
(event-button() != Qt::LeftButton);
otherwise opening a context-menu does not work anymore in the doubleclick
mode (or middleclicking is not possible).
dolphin/src/kitemviews/kitemlistcontroller.cpp
http://git.reviewboard.kde.org/r/102447/#comment5321
Should be something like:
const bool useDoubleClick = !KGlobalSettings::singleClick()
(event-buttons() Qt::LeftButton)
index = 0 index m_model-count();
if (useDoubleClick) {
to work correctly with the change done above.
Thanks, works nice! Please push it to master after fixing the two minor issues
above.
- Peter
On Aug. 26, 2011, 8:51 p.m., Tirtha Chatterjee wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102447/
---
(Updated Aug. 26, 2011, 8:51 p.m.)
Review request for KDE Base Apps and Peter Penz.
Summary
---
Honours the KGlobalSettings::singleClick() setting (set from Mouse KCM,
whether files should be opened on single or double clicks), and functions
accordingly.
Diffs
-
dolphin/src/kitemviews/kitemlistcontroller.cpp 29e2f47
Diff: http://git.reviewboard.kde.org/r/102447/diff
Testing
---
Yes tested and works.
Thanks,
Tirtha
Re: Review Request: Make Dolphin honour the KGlobalSettings::singleClick option.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102447/#review6050 --- Thanks for the update, but could you please revert the changes with KGlobalSettings::changeCursorOverIcon()? I consider this option an unnecessary micro-option and plan to implement the same behavior as in Dolphin 1.7. - Peter On Aug. 27, 2011, 7:31 a.m., Tirtha Chatterjee wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102447/ --- (Updated Aug. 27, 2011, 7:31 a.m.) Review request for KDE Base Apps and Peter Penz. Summary --- Honours the KGlobalSettings::singleClick() setting (set from Mouse KCM, whether files should be opened on single or double clicks), and functions accordingly. Diffs - dolphin/src/kitemviews/kitemlistcontroller.cpp 29e2f47 Diff: http://git.reviewboard.kde.org/r/102447/diff Testing --- Yes tested and works. Thanks, Tirtha
Re: Review Request: Make Dolphin honour the KGlobalSettings::singleClick option.
On Aug. 27, 2011, 9:31 a.m., Peter Penz wrote: Thanks for the update, but could you please revert the changes with KGlobalSettings::changeCursorOverIcon()? I consider this option an unnecessary micro-option and plan to implement the same behavior as in Dolphin 1.7. Tirtha Chatterjee wrote: Yeah sure. But TBH, it does feel a bit odd if a pointing hand cursor does not appear, and the folder opens on a single click. Reverting it, and committing for now. Thanks for pushing this! But TBH, it does feel a bit odd if a pointing hand cursor does not appear, and the folder opens on a single click We'll introduce the pointing hand cursor later, but: - not as an option - and first the selection markers need to be reintroduced again, as this will make the code for the pointing hand a little bit more complicated - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102447/#review6050 --- On Aug. 27, 2011, 7:31 a.m., Tirtha Chatterjee wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102447/ --- (Updated Aug. 27, 2011, 7:31 a.m.) Review request for KDE Base Apps and Peter Penz. Summary --- Honours the KGlobalSettings::singleClick() setting (set from Mouse KCM, whether files should be opened on single or double clicks), and functions accordingly. Diffs - dolphin/src/kitemviews/kitemlistcontroller.cpp 29e2f47 Diff: http://git.reviewboard.kde.org/r/102447/diff Testing --- Yes tested and works. Thanks, Tirtha
Re: Review Request: Find list items by typing their initial letters.
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102465/#review6075
---
Thanks for this patch. As discussed per e-mail already I think from a design
point of view this is fine and the patch looks good! I've added quite a lot of
nitpicking comments, would be great if you could fix those things and do one
update here. Afterwards I'm confident that this can be pushed.
dolphin/src/kitemviews/kfileitemmodel.cpp
http://git.reviewboard.kde.org/r/102465/#comment5351
Coding style - braces:
if (startFromIndex 0) {
startFromIndex = 0;
}
also
startFromIndex = qMax(0, startFromIndex)
would be an option.
dolphin/src/kitemviews/kfileitemmodel.cpp
http://git.reviewboard.kde.org/r/102465/#comment5352
Coding style - braces:
for (int i = startFromIndex; i count(); i++) {
...
dolphin/src/kitemviews/kfileitemmodel.cpp
http://git.reviewboard.kde.org/r/102465/#comment5353
Coding style - braces
dolphin/src/kitemviews/kitemlistcontroller.h
http://git.reviewboard.kde.org/r/102465/#comment5354
Please remove this signal, the searching is done internally and should not
be exposed to the public.
dolphin/src/kitemviews/kitemlistcontroller.h
http://git.reviewboard.kde.org/r/102465/#comment5355
- Change 'QString text' to 'const QString text'
- I'd suggest to rename it to: void requestItemActivationByKeyboard(const
QString text)
dolphin/src/kitemviews/kitemlistcontroller.cpp
http://git.reviewboard.kde.org/r/102465/#comment5356
Coding style - braces
dolphin/src/kitemviews/kitemlistcontroller.cpp
http://git.reviewboard.kde.org/r/102465/#comment5357
const int startFromIndex = ...
const int index = ...;
dolphin/src/kitemviews/kitemlistkeyboardmanager.h
http://git.reviewboard.kde.org/r/102465/#comment5358
I'm fine with your suggestion above to use the (quite long) name
KItemListKeyboardSearchManager. I think it makes clearer what is done here...
But please add a _p.h postfix to the name of the headerfile (-
kitemlistkeyboardsearchmanager_p.h) as it is meant as private class from a
module point of view.
dolphin/src/kitemviews/kitemlistkeyboardmanager.h
http://git.reviewboard.kde.org/r/102465/#comment5361
I think the name is quite confusing, I'd suggest to simply use:
void addKey(const QString key)
and add a documentation to the method what it does (also here: 'const
QString' instead of 'QString')
dolphin/src/kitemviews/kitemlistkeyboardmanager.h
http://git.reviewboard.kde.org/r/102465/#comment5359
Replace 'QString string' by 'const QString string'
dolphin/src/kitemviews/kitemlistkeyboardmanager.h
http://git.reviewboard.kde.org/r/102465/#comment5360
Personally I'd prefer using a QTimer* m_timer here, but this is fine too.
dolphin/src/kitemviews/kitemlistkeyboardmanager.cpp
http://git.reviewboard.kde.org/r/102465/#comment5362
KFileItemModel (or any other concrete implementation of the model) may not
be included in this class. Is not used anyhow here :-)
dolphin/src/kitemviews/kitemmodelbase.h
http://git.reviewboard.kde.org/r/102465/#comment5363
Please start each sentence in the documentation with a capital letter and
end it with a dot.
- Peter
On Aug. 27, 2011, 8:36 p.m., Tirtha Chatterjee wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102465/
---
(Updated Aug. 27, 2011, 8:36 p.m.)
Review request for KDE Base Apps and Peter Penz.
Summary
---
This patch allows finding items by typing on the keyboard while the
KItemListController is in focus. Timer and key queuing is handled by
KItemListKeyboardManager, and search itself is done by reimplementing
indexForKeyboardSearch(QString) from KItemModelBase. This implementation does
not, at the moment, take care of the repetitive typing as suggested by Frank.
I think we can implement that once this is in.
p.s. Not sure if line 213 in kitemlistcontroller.cpp is the best way to do
it. Returning true does not work.
p.s. I feel the name KItemListKeyboardManager can be changed to
KItemListKeyboardSearchManager, although a little too big.
Diffs
-
dolphin/src/CMakeLists.txt 31d3f89
dolphin/src/kitemviews/kfileitemmodel.h 654c7db
dolphin/src/kitemviews/kfileitemmodel.cpp f36ab83
dolphin/src/kitemviews/kitemlistcontroller.h 134e116
dolphin/src/kitemviews/kitemlistcontroller.cpp 2e32880
dolphin/src/kitemviews/kitemlistkeyboardmanager.h PRE-CREATION
dolphin/src/kitemviews/kitemlistkeyboardmanager.cpp PRE-CREATION
dolphin/src/kitemviews/kitemmodelbase.h 4670469
dolphin/src/kitemviews/kitemmodelbase.cpp fc604e7
Re: Review Request: Find list items by typing their initial letters.
On Aug. 27, 2011, 9:08 p.m., Peter Penz wrote: Thanks for this patch. As discussed per e-mail already I think from a design point of view this is fine and the patch looks good! I've added quite a lot of nitpicking comments, would be great if you could fix those things and do one update here. Afterwards I'm confident that this can be pushed. Ah, forgot regarding the coding style nitpicks - Dolphin tries to follow the coding-style in kdelibs: http://techbase.kde.org/Policies/Kdelibs_Coding_Style - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102465/#review6075 --- On Aug. 27, 2011, 8:36 p.m., Tirtha Chatterjee wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102465/ --- (Updated Aug. 27, 2011, 8:36 p.m.) Review request for KDE Base Apps and Peter Penz. Summary --- This patch allows finding items by typing on the keyboard while the KItemListController is in focus. Timer and key queuing is handled by KItemListKeyboardManager, and search itself is done by reimplementing indexForKeyboardSearch(QString) from KItemModelBase. This implementation does not, at the moment, take care of the repetitive typing as suggested by Frank. I think we can implement that once this is in. p.s. Not sure if line 213 in kitemlistcontroller.cpp is the best way to do it. Returning true does not work. p.s. I feel the name KItemListKeyboardManager can be changed to KItemListKeyboardSearchManager, although a little too big. Diffs - dolphin/src/CMakeLists.txt 31d3f89 dolphin/src/kitemviews/kfileitemmodel.h 654c7db dolphin/src/kitemviews/kfileitemmodel.cpp f36ab83 dolphin/src/kitemviews/kitemlistcontroller.h 134e116 dolphin/src/kitemviews/kitemlistcontroller.cpp 2e32880 dolphin/src/kitemviews/kitemlistkeyboardmanager.h PRE-CREATION dolphin/src/kitemviews/kitemlistkeyboardmanager.cpp PRE-CREATION dolphin/src/kitemviews/kitemmodelbase.h 4670469 dolphin/src/kitemviews/kitemmodelbase.cpp fc604e7 Diff: http://git.reviewboard.kde.org/r/102465/diff Testing --- yes. tested and works. Thanks, Tirtha
Re: Review Request: Allow externally deleted files to be removed from view in Dolphin
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102435/#review6020 --- Ship it! Thanks for the patch, works nice! I've added a small unit-test for it and have just pushed it to master. - Peter On Aug. 25, 2011, 7:23 p.m., Tirtha Chatterjee wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102435/ --- (Updated Aug. 25, 2011, 7:23 p.m.) Review request for KDE Base Apps and Peter Penz. Summary --- Currently, if a file is deleted by an external application, and the directory containing that file is open in Dolphin, the change is not reflected in Dolphin until we refresh. This patch fixes it. Diffs - dolphin/src/kitemviews/kfileitemmodel.cpp 189aa75 Diff: http://git.reviewboard.kde.org/r/102435/diff Testing --- Yes, tested locally. Thanks, Tirtha
Re: Review Request: Handle focus in KUrlNavigator
On Aug. 20, 2011, 3 p.m., Commit Hook wrote: This review has been submitted with commit ae3b7a48ec0e34ac64c5531ec45b1f898594898a by Peter Penz to branch KDE/4.7. Thanks for the update of the patch, it works nice now! I did some minor modifications (the button should not trigger the parent to repaint() itself etc.) but pushed your patch to KDE/4.7 and the frameworks branch. - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102345/#review5855 --- On Aug. 18, 2011, 6 p.m., José Millán Soto wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102345/ --- (Updated Aug. 18, 2011, 6 p.m.) Review request for kdelibs. Summary --- This patch makes KUrlNavigator focusable. Also, QStyle::drawControl is used instead of QPainter::drawText in KUrlNavigatorButton, because accelerators are set in the buttons and drawText did not display them correctly. Diffs - kfile/kurlnavigator.cpp e71c979 kfile/kurlnavigatorbutton.cpp 5d38e56 kfile/kurlnavigatorbutton_p.h 20ce117 kfile/kurlnavigatorbuttonbase.cpp 45f8eee kfile/kurlnavigatorbuttonbase_p.h 70aacb3 Diff: http://git.reviewboard.kde.org/r/102345/diff Testing --- Thanks, José
Review Request: Don't hang when determining MIME type of corrupted files
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102391/ --- Review request for kdelibs and David Faure. Summary --- If KMimeTypeRepository::findFromContent() tries to determine MIME from a file that cannot be read, such as on a corrupted optical disc, a read attempt is made in KMimeMagicMatch::match() for every available rule, resulting in UI hangs (e.g. file dialogs, dolphin) for tens of minutes (see https://bugs.kde.org/show_bug.cgi?id=280446 for more details). I've submitted this patch here on behalf of Miroslav ?os, who has submitted the bug-report and also has written the patch. This addresses bug 280446. http://bugs.kde.org/show_bug.cgi?id=280446 Diffs - kdecore/services/kmimetype.cpp 955bf62 kdecore/services/kmimetyperepository.cpp 6ff3d16 Diff: http://git.reviewboard.kde.org/r/102391/diff Testing --- Thanks, Peter
Re: Review Request: Handle focus in KUrlNavigator
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102345/#review5783 --- Thanks for the patch! I agree that the KUrlNavigator should be focusable but in the current form a few things are missing yet: - When focusing the buttons of the KUrlNavigator it should be possible to trigger them by pressing RETURN. - When focusing a button pressing Arrow down should open the sub-folders - From a visual point of view I don't like it that text-mnemonics are shown per default. I'd propose to show them only if the KUrlNavigator has the focus. Do you think you have the time to update the patch? If not I can take care for this myself but it will probably take some time (1 or 2 weeks). - Peter On Aug. 17, 2011, 5:37 p.m., José Millán Soto wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102345/ --- (Updated Aug. 17, 2011, 5:37 p.m.) Review request for kdelibs. Summary --- This patch makes KUrlNavigator focusable. Also, QStyle::drawControl is used instead of QPainter::drawText in KUrlNavigatorButton, because accelerators are set in the buttons and drawText did not display them correctly. Diffs - kfile/kurlnavigatorbuttonbase.cpp 45f8eee kfile/kurlnavigatorbuttonbase_p.h 70aacb3 kfile/kurlnavigator.cpp e71c979 kfile/kurlnavigatorbutton.cpp 5d38e56 Diff: http://git.reviewboard.kde.org/r/102345/diff Testing --- Thanks, José
Re: Review Request: Dolphin renaming functionality to include user choice in starting index number
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102328/#review5722
---
Ship it!
Thanks for the update, looks good! I've pushed your patch to master and only
did a minor adjustment regarding the layout, so that the box is aligned beside
the text.
- Peter
On Aug. 15, 2011, 1:16 p.m., Chirag Anand wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102328/
---
(Updated Aug. 15, 2011, 1:16 p.m.)
Review request for KDE Base Apps and Peter Penz.
Summary
---
This patch applies to dolphin/src/views/renamedialog.cpp and
dolphin/src/views/renamedialog.h. It is built on the current master
branch of kde-baseapps repository.
It replaces the current renaming functionality of Dolphin for multiple
files. The functionality gives user a little more control over how to
rename their files. It asks the user for the starting point of the
sequence of number instead of starting it always from 1. Renaming 3
files by doing FILE##.txt gives us FILE01.txt, FILE02.txt, FILE03.txt.
If we have to rename these 3 files to FILE04.txt, FILE05.txt,
FILE06.txt, we need 6 files for the operation. We cannot rename files
starting from a random number. Also, we cannot rename file with
different extensions in a sequence. Though this patch does not yet solve
this problem, what it does is to give the user a choice to start the
sequence from wherever he/she wants it to.
I have used parenthesis '(', ')' instead of '#' as placeholder. For the
above example, we have to rename the files by doing FILE(04).txt and it
will rename the files as required.
Diffs
-
dolphin/src/views/renamedialog.h 8d8b73da56c6675b4e81d94f7467e5a52e440c11
dolphin/src/views/renamedialog.cpp c0c6ad58c1153daed7c15b3f7be661fb39bffb4d
Diff: http://git.reviewboard.kde.org/r/102328/diff
Testing
---
Thanks,
Chirag
Re: Review Request: Dolphin's preview configuration doesn't support over 2 GB files.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102319/#review5683 --- Ship it! Thanks for the patch, looks fine! Please push it to master (if you don't have a git-account please let me know and I'll push it for you) - Peter On Aug. 13, 2011, 10:03 p.m., Jussi Judin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102319/ --- (Updated Aug. 13, 2011, 10:03 p.m.) Review request for KDE Base Apps. Summary --- Fix Dolphin's preview settings to support larger preview sizes than 2 GB in size. Reported as KDE bug 280034. Diffs - dolphin/src/settings/general/previewssettingspage.cpp 590a51d Diff: http://git.reviewboard.kde.org/r/102319/diff Testing --- Steps to Reproduce: Input megabytes to maximum preview size and save configuration. Then close configuration and go to the preview settings again and notice that the maximum preview size is not megabytes. Expected Results: Maximum preview size should be the one entered in the input box after saving Dolphin's configuration. Thanks, Jussi
Re: Review Request: Dolphin's preview configuration doesn't support over 2 GB files.
On Aug. 14, 2011, 10:31 a.m., Peter Penz wrote: Thanks for the patch, looks fine! Please push it to master (if you don't have a git-account please let me know and I'll push it for you) Jussi Judin wrote: No I don't so you need to do it. I've pushed it to master now :-) - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102319/#review5683 --- On Aug. 13, 2011, 10:03 p.m., Jussi Judin wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102319/ --- (Updated Aug. 13, 2011, 10:03 p.m.) Review request for KDE Base Apps. Summary --- Fix Dolphin's preview settings to support larger preview sizes than 2 GB in size. Reported as KDE bug 280034. Diffs - dolphin/src/settings/general/previewssettingspage.cpp 590a51d Diff: http://git.reviewboard.kde.org/r/102319/diff Testing --- Steps to Reproduce: Input megabytes to maximum preview size and save configuration. Then close configuration and go to the preview settings again and notice that the maximum preview size is not megabytes. Expected Results: Maximum preview size should be the one entered in the input box after saving Dolphin's configuration. Thanks, Jussi
Re: Review Request: fix for #277372 - dolphin part looses view state on every tab change
On July 11, 2011, 9 p.m., Peter Penz wrote: Thanks for the patch, looks good! Peter Penz wrote: Committed to 4.7 (is already fixed for Dolphin 2.0 that will get merged to master around beginning of August) Marcel Partap wrote: still nothing pushed to public repo. huh? (: Pushed to kde-baseapps to KDE/4.7 with commit c4321a1b1dead10be191b23b08f8b53983633b23 on the 12th of July. - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101919/#review4616 --- On July 11, 2011, 8:45 p.m., Marcel Partap wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101919/ --- (Updated July 11, 2011, 8:45 p.m.) Review request for KDE Base Apps, David Faure and Peter Penz. Summary --- At the point where m_viewModeController-setUrl(url) was invoked in current code, the view does not yet exist because it is only created in applyViewProperties(). Moving the call after view creation is not enough however. The DolphinView's url itself has to be set, that implies setting the url of the VMC. Correcting this makes the showEvent() hack unnecessary. Diffs - dolphin/src/views/dolphinview.h 48967e6 dolphin/src/views/dolphinview.cpp 681ce74 Diff: http://git.reviewboard.kde.org/r/101919/diff Testing --- Thanks, Marcel
Re: Review Request: DolphinDetailsView: fix column auto size fail on custom font styles
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101923/#review4618 --- Ship it! Thanks for the patch! It looks good, but it only makes sense to push it to the 4.7 branch: For master Dolphin 2.0 will be integrated until beginning of August which has a new view-engine that makes this patch obsolete. If you plan to do further fixes for the icons-, details- or column-view (or related classes) please contact me directly before as most probably the patches are not compatible with the new view-engine. - Peter On July 11, 2011, 9:25 p.m., Marcel Partap wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101923/ --- (Updated July 11, 2011, 9:25 p.m.) Review request for KDE Base Apps, David Faure and Peter Penz. Summary --- The auto-calculated width of columns always is same regardless of custom font style so there was sure something wrong.. = Setting the font in the viewOptions() actually has no effect on either viewport or view, has to be done via setFont(). So - adding setFont(m_font) in view init phase - removing font stuff from viewOptions() and NOOP assignment in slotGlobalSettingsChanged() - avoiding extra inter-object calls by using m_font directly Also replaced abusing fontMetrics.height for horizontalGap - by coincidence it was more then needed, but for bigger fonts might be 20px and up while usually only 10px is needed. Pixel-perfect calculation required DEEP tracing throughout QT style rendering stuff - (PM_FocusFrameHMargin+1)*2 seems to be the way it is calculated, +const 4 which is hard-coded all over the place though. Diffs - dolphin/src/views/dolphindetailsview.cpp 0ce26df Diff: http://git.reviewboard.kde.org/r/101923/diff Testing --- cgdb tracing aaall day long ^^ Thanks, Marcel
Re: Review Request: fix #277269 Dolphin(Part) Detail/Tree view, highlighted selection paint glitch
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101924/#review4620 --- dolphin/src/views/dolphintreeview.cpp http://git.reviewboard.kde.org/r/101924/#comment4049 Memory leak: QWidget::setStyle() does not transfer the ownership... - Peter On July 11, 2011, 9:39 p.m., Marcel Partap wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101924/ --- (Updated July 11, 2011, 9:39 p.m.) Review request for KDE Base Apps, David Faure and Peter Penz. Summary --- What was strange that background highlighting and actual item selection were drawn independently from each other and the bogus highlighting to the left of the item was not cleared... Now this one was a __REAL__ bitch to get dealt with, took me hours and hours bashing my head against the shell ^^ ok now again the viewOptions is not only not the place to turn off background highlighting, but there it was even tried to ENABLE it :O turned out this so called QStyle::SH_ItemView_ShowDecorationSelected documented as When an item in an item view is selected, also highlight the branch or other decoration. is hard-coded on by DEFAULT in QCommonStyle and all inheriting from there so it requires a QProxyStyle to override the setting. While we have the opportunity, also set SH_ItemView_ArrowKeysNavigateIntoChildren for added joice of keyboard navigation (although strange effect comes up when being on a leaf and pressing Cursor::Right again - but with or without this setting, something with the selection handler...) ...now someone owes me CAKE for this one :D Diffs - dolphin/src/views/dolphindetailsview.cpp 0ce26df dolphin/src/views/dolphintreeview.h c037d41 dolphin/src/views/dolphintreeview.cpp 64b66aa Diff: http://git.reviewboard.kde.org/r/101924/diff Testing --- head-bashing Screenshots --- dolphin-treeview-selection-paint-fail http://git.reviewboard.kde.org/r/101924/s/195/ Thanks, Marcel
Re: Review Request: fix #277269 Dolphin(Part) Detail/Tree view, highlighted selection paint glitch
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101924/#review4621 --- Thanks again for your investigations. Like for the previous patch: It would only make sense to push it to the 4.7 branch. In this case I'm a little bit concerned to backport such a non-trivial change, but I trust you here. Please do a careful testing especially with the Oxygen-style as this style is used by most people. After fixing the leak I'm fine if this patch gets pushed to the 4.7 branch. - Peter On July 11, 2011, 9:39 p.m., Marcel Partap wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101924/ --- (Updated July 11, 2011, 9:39 p.m.) Review request for KDE Base Apps, David Faure and Peter Penz. Summary --- What was strange that background highlighting and actual item selection were drawn independently from each other and the bogus highlighting to the left of the item was not cleared... Now this one was a __REAL__ bitch to get dealt with, took me hours and hours bashing my head against the shell ^^ ok now again the viewOptions is not only not the place to turn off background highlighting, but there it was even tried to ENABLE it :O turned out this so called QStyle::SH_ItemView_ShowDecorationSelected documented as When an item in an item view is selected, also highlight the branch or other decoration. is hard-coded on by DEFAULT in QCommonStyle and all inheriting from there so it requires a QProxyStyle to override the setting. While we have the opportunity, also set SH_ItemView_ArrowKeysNavigateIntoChildren for added joice of keyboard navigation (although strange effect comes up when being on a leaf and pressing Cursor::Right again - but with or without this setting, something with the selection handler...) ...now someone owes me CAKE for this one :D Diffs - dolphin/src/views/dolphindetailsview.cpp 0ce26df dolphin/src/views/dolphintreeview.h c037d41 dolphin/src/views/dolphintreeview.cpp 64b66aa Diff: http://git.reviewboard.kde.org/r/101924/diff Testing --- head-bashing Screenshots --- dolphin-treeview-selection-paint-fail http://git.reviewboard.kde.org/r/101924/s/195/ Thanks, Marcel
Re: Review Request: Add missing actions to report bug + switch language to Help menu in dolphin whithout menubar
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101597/#review3852 --- Ship it! Thanks for the patch! I completely missed this entry (I rarely report Dolphin bugs this way ;-))... - Peter On June 12, 2011, 8:21 p.m., Burkhard Lück wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101597/ --- (Updated June 12, 2011, 8:21 p.m.) Review request for KDE Base Apps and Peter Penz. Summary --- Using Dolphin in default mode in master /4.7.) whithout menubar the user has no actions to report a bug or switch language. Patch adds this to the Help menu launched from the toolbar button Configure and control Dolphin. Diffs - dolphin/src/dolphinmainwindow.cpp 198e2da Diff: http://git.reviewboard.kde.org/r/101597/diff Testing --- Thanks, Burkhard
Re: Review Request: Draw overlays even for previews.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101570/#review3831 --- Ship it! Looks fine, please commit! - Peter On June 10, 2011, 7:46 p.m., Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101570/ --- (Updated June 10, 2011, 7:46 p.m.) Review request for kdelibs, David Faure and Peter Penz. Summary --- This way it is easier to recognise links to images etc. Depends on https://git.reviewboard.kde.org/r/101569/ This addresses bug 190579. http://bugs.kde.org/show_bug.cgi?id=190579 Diffs - kfile/kfilepreviewgenerator.cpp 216dd7e Diff: http://git.reviewboard.kde.org/r/101570/diff Testing --- Screenshots --- http://git.reviewboard.kde.org/r/101570/s/179/ Thanks, Matthias
Re: Review Request: Show icon overlays in the Informationen Panel.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101573/#review3832 --- Ship it! Thanks for the patch! Please commit if the kdelibs-patch is pushed on Monday. - Peter On June 10, 2011, 7:52 p.m., Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101573/ --- (Updated June 10, 2011, 7:52 p.m.) Review request for KDE Base Apps and Peter Penz. Summary --- Depends on https://git.reviewboard.kde.org/r/101569/ This addresses bug 190579. http://bugs.kde.org/show_bug.cgi?id=190579 Diffs - dolphin/src/panels/information/informationpanelcontent.cpp 77a6232 Diff: http://git.reviewboard.kde.org/r/101573/diff Testing --- Thanks, Matthias
Re: Review Request: Return the url of the view instead of the url of the url navigator.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101580/#review3833 --- Ship it! Ah, very nice corner case :-) Patch is fine, thanks! - Peter On June 10, 2011, 10:11 p.m., Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101580/ --- (Updated June 10, 2011, 10:11 p.m.) Review request for KDE Base Apps and Peter Penz. Summary --- That way if a wrong protocol had been entered the currently watched directory will be returned. This addresses bug 274890. http://bugs.kde.org/show_bug.cgi?id=274890 Diffs - dolphin/src/dolphinviewcontainer.cpp 1042ece Diff: http://git.reviewboard.kde.org/r/101580/diff Testing --- Thanks, Matthias
Re: Review Request: DolphinColumnView navigation works more intuitively.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101449/#review3550 --- dolphin/src/views/dolphincolumnview.cpp http://git.reviewboard.kde.org/r/101449/#comment2972 Minor cosmetic nitpick: Please use the // comments instead of the /* ... */ within the code. - Peter On May 27, 2011, 4:04 p.m., Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101449/ --- (Updated May 27, 2011, 4:04 p.m.) Review request for KDE Base Apps and Peter Penz. Summary --- If no item is selected then pressing right moves to a column view with child url, instead of the first index. This addresses bug 263110. http://bugs.kde.org/show_bug.cgi?id=263110 Diffs - dolphin/src/views/dolphincolumnview.h be50ea3 dolphin/src/views/dolphincolumnview.cpp fa1f620 Diff: http://git.reviewboard.kde.org/r/101449/diff Testing --- Thanks, Matthias
Re: Review Request: DolphinColumnView navigation works more intuitively.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101449/#review3551 --- Ship it! Thanks for the patch! Looks good, please push it to master :-) - Peter On May 27, 2011, 4:04 p.m., Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101449/ --- (Updated May 27, 2011, 4:04 p.m.) Review request for KDE Base Apps and Peter Penz. Summary --- If no item is selected then pressing right moves to a column view with child url, instead of the first index. This addresses bug 263110. http://bugs.kde.org/show_bug.cgi?id=263110 Diffs - dolphin/src/views/dolphincolumnview.h be50ea3 dolphin/src/views/dolphincolumnview.cpp fa1f620 Diff: http://git.reviewboard.kde.org/r/101449/diff Testing --- Thanks, Matthias
Re: Review Request:
On May 27, 2011, 10:43 p.m., Frank Reininghaus wrote: If Rename Inline is enabled, DolphinView::renameSelectedItems() calls m_viewAccessor.itemView()-edit(proxyIndex), which then opens the editor in KFileItemDelegate. When the editing is done, KFileItemDelegate::setModelData(...) is called, which calls KDirModel::setData(...), where the actual file renaming is triggered. Maybe the check could be done there, but you might want to ask David Faure about that. Ah, we've answered in parallel :-) - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101454/#review3553 --- On May 27, 2011, 10:19 p.m., Matthias Fuchs wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101454/ --- (Updated May 27, 2011, 10:19 p.m.) Review request for KDE Base Apps and Peter Penz. Summary --- So far this works only for the RenameDialog as I could not find the method that changes the name when inline renaming is enabled. If you could tell me where that method is I would in fact adapt the patch. This addresses bug 211751. http://bugs.kde.org/show_bug.cgi?id=211751 Diffs - dolphin/src/views/renamedialog.cpp 810562a Diff: http://git.reviewboard.kde.org/r/101454/diff Testing --- Thanks, Matthias
Re: Review Request: Fix directory navigation in Dolphin::Terminal.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101096/#review2571 --- Ship it! Thanks, looks good! dolphin/src/panels/terminal/terminalpanel.cpp http://git.reviewboard.kde.org/r/101096/#comment2249 Please leave the open/closing braces also when only one statement is used (see http://techbase.kde.org/Policies/Kdelibs_Coding_Style) - Peter On April 12, 2011, 4:37 a.m., Raphael Kubo da Costa wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101096/ --- (Updated April 12, 2011, 4:37 a.m.) Review request for KDE Base Apps. Summary --- Fix directory navigation in Dolphin::Terminal. When navigating in Dolphin it attempts to keep any open Terminal (F4) in sync by changing the directory in the shell. It does this by sending a ^C; cd $DIRECTORY however shells under FreeBSD treat ^C as a literal string and not SIGINT. Fix this by sending SIGINT to the shell instead of ^C. It appears Linux does not exhibit this behaviour. Patch originally written by David Naylor, from the KDE-FreeBSD team. Diffs - dolphin/src/panels/terminal/terminalpanel.cpp 61d80cbfa26fb17d0ba403a52f2ab57fbae7b3cc Diff: http://git.reviewboard.kde.org/r/101096/diff Testing --- Changing directories with a terminal open now changes the terminal's current directory fine on FreeBSD. Thanks, Raphael
Re: Review Request: Set the properties action fom mainWindow actionCollection
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101076/#review2551 --- Ship it! Thanks, looks good! - Peter On April 10, 2011, 9:54 a.m., Alex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101076/ --- (Updated April 10, 2011, 9:54 a.m.) Review request for KDE Base Apps. Summary --- In order to have a proper kiosk support in dolphin we should use standard actions whether we can. Dolphin does it right but a few exceptions, this try to fix one of those. the properties action from items is made by getting it from the mainWindow::actionCollection but the properties action from openViewportContextMenu is custom, which bypasses kiosk. The patch simply replace the custom properties action and uses the mainWindow::actionCollection one. Diffs - dolphin/src/dolphincontextmenu.cpp 0aa82b2 Diff: http://git.reviewboard.kde.org/r/101076/diff Testing --- Compare the result of clicking on the Properties custom action and the standard one. Thanks, Alex
Re: Review Request: Dolphin details view optimization
On March 10, 2011, 8:05 a.m., Peter Penz wrote: Thanks a lot for this good patch, please commit :-) Would also be great if this could be backported to 4.6.2. I've committed the patch and backported it to 4.6. - Peter --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100826/#review1879 --- On March 9, 2011, 11:38 p.m., Samuel Rødal wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100826/ --- (Updated March 9, 2011, 11:38 p.m.) Review request for KDE Base Apps. Summary --- Constructing a KColorScheme object is very expensive because of a number of tint computations. When scrolling a big list more than 30 % of the time was spent here. Instead, we can precompute and store the inactive text color. Diffs - dolphin/src/views/dolphinfileitemdelegate.h 5eb559a7909a50afef8a67a8706ac3106dab38bb dolphin/src/views/dolphinfileitemdelegate.cpp 9fed95bca2a8170ddbc18239a990ce8a28528bf4 Diff: http://git.reviewboard.kde.org/r/100826/diff Testing --- Thanks, Samuel
Review Request: Allow to configure thumbnail-plugins
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100717/ --- Review request for kdelibs and David Faure. Summary --- The patch allows to make thumbnail-plugins configurable by the user. One example is the JPEG-thumbnail-plugin: Some users prefer that the preview gets rotated automatically corresponding to the EXIF-information, for some users the automatic-rotating is an issue (e.g. they want to see in Dolphin/Konqueror whether the image needs to get rotated manually). In KDE SC 4.6 we tried to bypass this issue by offering 2 plugins: One JPEG-thumbnail-plugin for unrotated JPEGs and one for automatically-rotated JPEGS. But this is just a hack as when enabling both plugins it is undetermined which plugin will be used... Diffs - kio/kio/thumbcreator.h 51234e7 kio/kio/thumbcreator.cpp 4384814 Diff: http://git.reviewboard.kde.org/r/100717/diff Testing --- The patch has been tested by implementing a configuration option for the JPEG-thumbnail-plugin (see screenshot). Screenshots --- Configure JPEG-thumbnail-plugin http://git.reviewboard.kde.org/r/100717/s/82/ Thanks, Peter
Re: Review Request: Enlarge image in folder preview when there's only one image
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6332/#review9634 --- Looks good from my point of view! I'd still suggest to wait a few days with committing the patch to collect further feedback (I'm not the maintainer of the class, I just did a few fixes/adjustments there). - Peter On Jan. 15, 2011, 9:37 a.m., Martin Engelmann wrote: --- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6332/ --- (Updated Jan. 15, 2011, 9:37 a.m.) Review request for kdelibs. Summary --- As stated in the bug report, the image on the folder preview should be enlarged if there is only one image. To achieve this there are two images created for the folder: img as before and oneTileImg. oneTileImg starts with the same image as img, but after the first thumbnail is generated oneTileImg won't be touched. The number of successfully created thumbnails is counted in validThumbnails. If only one thumbnail could be generated, oneTileImg is returned. The code between lines 566 and 589 in the original thumbnail.cpp was partly extracted into a new function called drawSubThumbnail. This is done to simplify the creation of the second preview. The extra effort to create the oneTileImg comparable to creating a fifth preview image for the folder. This addresses bug 240861. https://bugs.kde.org/show_bug.cgi?id=240861 Diffs - /trunk/KDE/kdebase/runtime/kioslave/thumbnail/thumbnail.h 1214477 /trunk/KDE/kdebase/runtime/kioslave/thumbnail/thumbnail.cpp 1214477 Diff: http://svn.reviewboard.kde.org/r/6332/diff Testing --- Tested using both Dolphin and Konqueror from trunk. The thumbnail generation was tested by moving images and files without thumbnail plug-in to a new folder. Then one file after another was deleted until only files without thumbnail plug-in were present. Also I opened randomly some folders with sub-folders containing large amounts of images (e.g. oxygen icon set with 550 action icons) to the performance. Screenshots --- Folder thumbnails for oxygen icon set in Konqueror http://svn.reviewboard.kde.org/r/6332/s/604/ Thanks, Martin
Re: Review Request: Use UDS_NAME as a fallback when sorting via UDS_DISPLAY_NAME
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6322/#review9602
---
trunk/KDE/kdelibs/kfile/kdirsortfilterproxymodel.cpp
http://svn.reviewboard.kde.org/r/6322/#comment10597
For performance reasons I'd suggest to implement it this way:
const int result = d-compare(leftFileItem.text(), rightFileItem.text(),
sortCaseSensitivity());
if (result == 0) {
return d-compare(leftFileItem.name(sortCaseSensitivity() ==
Qt::CaseInsensitive), rightFileItem.name(sortCaseSensitivity() ==
Qt::CaseInsensitive), sortCaseSensitivity()) 0;
} else {
return result 0;
}
By this we prevent doing another string comparison for equality for the
default case.
Otherwise I'm fine with the change!
- Peter
On Jan. 10, 2011, 10:24 a.m., Sebastian Trueg wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6322/
---
(Updated Jan. 10, 2011, 10:24 a.m.)
Review request for kdelibs and Peter Penz.
Summary
---
When sorting by name KDirSortFilterProxyModel uses UDS_DISPLAY_NAME. The
latter, however, is not unique. This results in strange GUI behaviour like
swapping items. This patch makes the model fall back to UDS_NAME to ensure a
fixed sort order.
I also propose to backport it to 4.6.
Diffs
-
trunk/KDE/kdelibs/kfile/kdirsortfilterproxymodel.cpp 1211983
Diff: http://svn.reviewboard.kde.org/r/6322/diff
Testing
---
Thanks,
Sebastian
Review Request: KFilePlacesView: Allow to add custom actions to the context menu
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/6267/ --- Review request for kdelibs and Kevin Ottens. Summary --- For KDE SC 4.7 Dolphin allows to lock/unlock the panels (= QDockWidgets) like in Amarok. Beside having a menu entry in the menu under View the dual-action Lock Panels/Unlock Panels is also provided as context menu entry for all panels. All panels have this menu-entry already, but the places panel is implemented as KFilePlacesView which currently does not allow to add custom actions to the context menu. The attached patch extends KFilePlacesView by this (see screenshot, where the action Unlock Panels has been added). Diffs - /trunk/KDE/kdelibs/kfile/kfileplacesview.h 1211329 /trunk/KDE/kdelibs/kfile/kfileplacesview.cpp 1211329 Diff: http://svn.reviewboard.kde.org/r/6267/diff Testing --- Tested in Dolphin by adding the actions Unlock Panels/Lock Panels. Screenshots --- Context menu with custom action Unlock Panels http://svn.reviewboard.kde.org/r/6267/s/600/ Thanks, Peter
