Re: Review Request 108727: ktimezoned: Watch /etc/localtime if it doesn't exist yet.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108727/#review27845 --- Ship it! I'm sorry this has taken so long - I've been incredibly busy recently. The patch looks fine. The only thing which occurs to me is to wonder whether the same scenario could happen with /etc/timezone (used by BSD) - if so, that file's creation should really be checked for as well as /etc/localtime. Without knowing whether that is actually possible, I think it might be worth adding the following comment where the patch sets mLocalIdFile: If under BSD it is possible for /etc/localtime to be missing but created later, we should also watch for its creation. - David Jarvie On Feb. 3, 2013, 4:30 a.m., Kevin Kofler wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108727/ --- (Updated Feb. 3, 2013, 4:30 a.m.) Review request for KDE Runtime and David Jarvie. Description --- /etc/localtime legitimately might not exist. The default is then UTC. But the file can then be created later, so watch for its creation. If we don't do this, when setting the time zone for the first time using kcm_clock, the initially set time zone will fail to get reloaded and the dialog will unexpectedly jump back to UTC. This problem shows up on Fedora 18, see: https://bugzilla.redhat.com/show_bug.cgi?id=906972 Please note that to test the fix with kcm_clock, you also need the kcm_clock (kde-workspace) fix from: https://git.reviewboard.kde.org/r/108711/ (which is already approved and which I'll push to KDE/4.10 and merge to master as soon as the 4.10.0 tagging freeze is lifted). Diffs - ktimezoned/ktimezoned.cpp 4eafa4e Diff: http://git.reviewboard.kde.org/r/108727/diff/ Testing --- Builds against at least 4.10.0 and 4.9.5. Works at runtime (and appears to fix the bug): https://bugzilla.redhat.com/show_bug.cgi?id=906972#c5 Thanks, Kevin Kofler
Re: [kdelibs] kio/kfile: completeBaseName gives us foo.bar for foo.bar.png
Hi ..
I pushed this set of changes into master by accident, after having done the
development in a local branch. Wasn't thinking .. it's also in KDE/4.10 branch
at this point as well.
(Well, I was thinking - I will have to pull this into the stable branch and
then into master .. ah, right, kdelibs is not doing the normal multiple branch
thing. *brain fart* master it is, then. *sigh*)
I merged KDE/4.10 back into master to ensure it wouldn't problems .. it went
ok, though now master has a slightly odd stutter in its commit msgs (though
all the code and changes are at least accurate).
I can't wait until we cut over to the frameworks branch for all devel. :/
On Thursday, February 21, 2013 18:23:54 Aaron Seigo wrote:
Git commit 6b1c9f53c19d2c9b129ff359f8fb1df676e25028 by Aaron Seigo.
Committed on 21/02/2013 at 18:11.
Pushed by aseigo into branch 'master'.
completeBaseName gives us foo.bar for foo.bar.png
icon names with dots in them were getting mangled as a result
BUG:315578
M +2-2kio/kfile/kicondialog.cpp
http://commits.kde.org/kdelibs/6b1c9f53c19d2c9b129ff359f8fb1df676e25028
diff --git a/kio/kfile/kicondialog.cpp b/kio/kfile/kicondialog.cpp
index b7d646f..63f4bd9 100644
--- a/kio/kfile/kicondialog.cpp
+++ b/kio/kfile/kicondialog.cpp
@@ -661,8 +661,8 @@ void KIconDialog::slotOk()
{
name = d-mpCanvas-getCurrent();
if (!name.isEmpty() d-mpSystemIcons-isChecked()) {
-QFileInfo fi(name);
-name = fi.baseName();
+const QFileInfo fi(name);
+name = fi.completeBaseName();
}
}
--
Aaron Seigo
signature.asc
Description: This is a digitally signed message part.
Re: Review Request 109049: Fix favicon support for chrome bookmarks on krunner
On Feb. 20, 2013, 1:06 a.m., Àlex Fiestas wrote: I guess this will break compatibility with old versions then? if so, do you know from which version will it work? If this was not long ago, can we check the version and keep supporting the old and the new one? Àlex Fiestas wrote: Code-wise the patch looks fine. Marco Gulino wrote: You're right, there's a compatibility break, and we can't even know starting from which version, since it's an internal database not documented (I might try looking around, but I don't expect to find much). If it makes sense, I might conditionally choose the right query depending on the schema found (assuming the new table didn't exist in the previous version, which makes sense). I also noticed that the new table is meant to support multiple icon sizes, so I might add an order by clause to choose the wider one as default. Àlex Fiestas wrote: To be clear, personally I'd be fine if we drop support for some old Chromium version, thing is those guys bump versions like crazy (today we are at chromium 20 and tomorrow we are at 30), so we need to know how old is this change to be able to decide if we want to drop support for older versions. Marco Gulino wrote: Hopefully not :P I think I found the commit date on chromium git server: http://git.chromium.org/gitweb/?p=git/chromium.git;a=commit;h=eded41c5c28bf94e128b6ab4cdd8030fb8b6d2f3 The commit date is august 2012, so maybe it's a september/october release? Àlex Fiestas wrote: For 4.11 we can drop it, but not for 4.10. Ideal solution will be supporting both though, this seems like a recent change. Also, this up to the maintainer (I'm not) but supporting both will always be preferred :p I had to add some indirection, as the FaviconFromBlob didn't have the database for querying the presence of our table. With the new patch we can query the sqlite database, and choose the right query if the favicon_bitmaps table is there, supporting both versions. I also added the order by clause for multi-favicon support. As for the maintainer, I don't know if there's an official maintainer for this runner in particular, but I rewrote most of it to support chrome, so probably it's just up to me :) - Marco --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109049/#review27760 --- On Feb. 19, 2013, 10:42 p.m., Marco Gulino wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109049/ --- (Updated Feb. 19, 2013, 10:42 p.m.) Review request for kde-workspace and Plasma. Description --- As reported by an user ( https://bugs.kde.org/show_bug.cgi?id=305633 ), chrome bookmarks database changed, and favicon wasn't shown anymore (not either the default star icon). I added the functionality back, and added a safety guard for displaying the default icon if something similar happens again. (note: I didn't set the bugs field here, since that bug was already closed, and was about something else). Diffs - plasma/generic/runners/bookmarks/faviconfromblob.cpp 93c720c Diff: http://git.reviewboard.kde.org/r/109049/diff/ Testing --- with chrome as default browser, install the plugin, restart krunner, type bookmarks to view all bookmarks: proper favicon is shown. Removing the database query fix, but leaving the safety guard, and cleaning favicon cache (to have again a broken feature case), the default icon is shown. Thanks, Marco Gulino
Re: Review Request 109049: Fix favicon support for chrome bookmarks on krunner
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109049/ --- (Updated Feb. 21, 2013, 7:29 p.m.) Review request for kde-workspace and Plasma. Description --- As reported by an user ( https://bugs.kde.org/show_bug.cgi?id=305633 ), chrome bookmarks database changed, and favicon wasn't shown anymore (not either the default star icon). I added the functionality back, and added a safety guard for displaying the default icon if something similar happens again. (note: I didn't set the bugs field here, since that bug was already closed, and was about something else). Diffs (updated) - plasma/generic/runners/bookmarks/faviconfromblob.h cff4835 plasma/generic/runners/bookmarks/faviconfromblob.cpp 93c720c plasma/generic/runners/bookmarks/fetchsqlite.h 8b181df plasma/generic/runners/bookmarks/fetchsqlite.cpp 871deff Diff: http://git.reviewboard.kde.org/r/109049/diff/ Testing --- with chrome as default browser, install the plugin, restart krunner, type bookmarks to view all bookmarks: proper favicon is shown. Removing the database query fix, but leaving the safety guard, and cleaning favicon cache (to have again a broken feature case), the default icon is shown. Thanks, Marco Gulino
Re: Review Request 109049: Fix favicon support for chrome bookmarks on krunner
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109049/#review27876 --- Ship it! Tested the patch with chromiium 24.0.1312.70 (181759) worked fine. Code wise it looks fine as well. - Àlex Fiestas On Feb. 21, 2013, 7:29 p.m., Marco Gulino wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109049/ --- (Updated Feb. 21, 2013, 7:29 p.m.) Review request for kde-workspace and Plasma. Description --- As reported by an user ( https://bugs.kde.org/show_bug.cgi?id=305633 ), chrome bookmarks database changed, and favicon wasn't shown anymore (not either the default star icon). I added the functionality back, and added a safety guard for displaying the default icon if something similar happens again. (note: I didn't set the bugs field here, since that bug was already closed, and was about something else). Diffs - plasma/generic/runners/bookmarks/faviconfromblob.h cff4835 plasma/generic/runners/bookmarks/faviconfromblob.cpp 93c720c plasma/generic/runners/bookmarks/fetchsqlite.h 8b181df plasma/generic/runners/bookmarks/fetchsqlite.cpp 871deff Diff: http://git.reviewboard.kde.org/r/109049/diff/ Testing --- with chrome as default browser, install the plugin, restart krunner, type bookmarks to view all bookmarks: proper favicon is shown. Removing the database query fix, but leaving the safety guard, and cleaning favicon cache (to have again a broken feature case), the default icon is shown. Thanks, Marco Gulino
Re: Review Request 109049: Fix favicon support for chrome bookmarks on krunner
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109049/#review27881 --- This review has been submitted with commit 9fb3c1daa5879f30c29849ac9f2132ac6a186640 by Marco Gulino to branch KDE/4.10. - Commit Hook On Feb. 21, 2013, 7:29 p.m., Marco Gulino wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109049/ --- (Updated Feb. 21, 2013, 7:29 p.m.) Review request for kde-workspace and Plasma. Description --- As reported by an user ( https://bugs.kde.org/show_bug.cgi?id=305633 ), chrome bookmarks database changed, and favicon wasn't shown anymore (not either the default star icon). I added the functionality back, and added a safety guard for displaying the default icon if something similar happens again. (note: I didn't set the bugs field here, since that bug was already closed, and was about something else). Diffs - plasma/generic/runners/bookmarks/faviconfromblob.h cff4835 plasma/generic/runners/bookmarks/faviconfromblob.cpp 93c720c plasma/generic/runners/bookmarks/fetchsqlite.h 8b181df plasma/generic/runners/bookmarks/fetchsqlite.cpp 871deff Diff: http://git.reviewboard.kde.org/r/109049/diff/ Testing --- with chrome as default browser, install the plugin, restart krunner, type bookmarks to view all bookmarks: proper favicon is shown. Removing the database query fix, but leaving the safety guard, and cleaning favicon cache (to have again a broken feature case), the default icon is shown. Thanks, Marco Gulino
Re: Review Request 109049: Fix favicon support for chrome bookmarks on krunner
On Feb. 22, 2013, 12:41 a.m., Àlex Fiestas wrote: Tested the patch with chromiium 24.0.1312.70 (181759) worked fine. Code wise it looks fine as well. Thanks! Applied to branches KDE/4.10, and master, I guess it's enough. - Marco --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109049/#review27876 --- On Feb. 21, 2013, 7:29 p.m., Marco Gulino wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109049/ --- (Updated Feb. 21, 2013, 7:29 p.m.) Review request for kde-workspace and Plasma. Description --- As reported by an user ( https://bugs.kde.org/show_bug.cgi?id=305633 ), chrome bookmarks database changed, and favicon wasn't shown anymore (not either the default star icon). I added the functionality back, and added a safety guard for displaying the default icon if something similar happens again. (note: I didn't set the bugs field here, since that bug was already closed, and was about something else). Diffs - plasma/generic/runners/bookmarks/faviconfromblob.h cff4835 plasma/generic/runners/bookmarks/faviconfromblob.cpp 93c720c plasma/generic/runners/bookmarks/fetchsqlite.h 8b181df plasma/generic/runners/bookmarks/fetchsqlite.cpp 871deff Diff: http://git.reviewboard.kde.org/r/109049/diff/ Testing --- with chrome as default browser, install the plugin, restart krunner, type bookmarks to view all bookmarks: proper favicon is shown. Removing the database query fix, but leaving the safety guard, and cleaning favicon cache (to have again a broken feature case), the default icon is shown. Thanks, Marco Gulino
