DevHeads.net

Review Request: Change the way favicons are stored in bookmarks.xml file

Review request for kdelibs.

Summary
Currently kde bookmarks keep the favicons in a <bookmark:icon> tag inside the <meta> and that is why the bookmarks.xml file cannot pass the xmllint check.

This patch proposes a simple change -- favicons are stored as a `favicon' attribute inside the <meta> tag. That change doesn't change any functionality but it enables bookmarks.xml to be in written correct xml.

Rationale: Why do I care about correct xml? Well, first of all it is nicer to have correct xml, but more importantly, I'm using syncplaces (<a href="http://www.andyhalford.com/syncplaces/" title="http://www.andyhalford.com/syncplaces/">http://www.andyhalford.com/syncplaces/</a>) addon for firefox to synchronize firefox and konqueror/rekonq bookmarks. If bookmarks are not written in correct xml, the plugin shows some errors and is unable to import & merge the bookmarks. With this little change everything works like a charm.

Diffs
kio/bookmarks/kbookmark.cc 912052e

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

Testing
I built kdelibs with this patch and I'm running them on my Kubuntu box for a week or so. You can access my patched kdelibs in my ppa on launchpad if you wish: <a href="https://launchpad.net/~brcha/+archive/ppa" title="https://launchpad.net/~brcha/+archive/ppa">https://launchpad.net/~brcha/+archive/ppa</a>

Up until now I didn't encounter any problems with this patch. I did reload all favicons, remove bookmarks, and resync them from firefox and everything worked out fine.

Thanks,

Filip

Comments

Re: Review Request 101815: Change the way favicons are stored in

By Filip Brcic at 10/27/2013 - 14:59

(Updated Oct. 27, 2013, 6:59 p.m.)

Status
This change has been discarded.

Review request for kdelibs.

Repository: kdelibs

Description
Currently kde bookmarks keep the favicons in a <bookmark:icon> tag inside the <meta> and that is why the bookmarks.xml file cannot pass the xmllint check.

This patch proposes a simple change -- favicons are stored as a `favicon' attribute inside the <meta> tag. That change doesn't change any functionality but it enables bookmarks.xml to be in written correct xml.

Rationale: Why do I care about correct xml? Well, first of all it is nicer to have correct xml, but more importantly, I'm using syncplaces (<a href="http://www.andyhalford.com/syncplaces/" title="http://www.andyhalford.com/syncplaces/">http://www.andyhalford.com/syncplaces/</a>) addon for firefox to synchronize firefox and konqueror/rekonq bookmarks. If bookmarks are not written in correct xml, the plugin shows some errors and is unable to import & merge the bookmarks. With this little change everything works like a charm.

Diffs
kio/bookmarks/kbookmark.cc 912052e

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

Testing
I built kdelibs with this patch and I'm running them on my Kubuntu box for a week or so. You can access my patched kdelibs in my ppa on launchpad if you wish: <a href="https://launchpad.net/~brcha/+archive/ppa" title="https://launchpad.net/~brcha/+archive/ppa">https://launchpad.net/~brcha/+archive/ppa</a>

Up until now I didn't encounter any problems with this patch. I did reload all favicons, remove bookmarks, and resync them from firefox and everything worked out fine.

Thanks,

Filip Brcic

Re: Review Request: Change the way favicons are stored in bookma

By David Faure at 08/30/2011 - 04:37

Looking at old emails, it seems the icon element is simply missing from the DTD.

Look for icon in <a href="http://xbel.sourceforge.net/language/versions/1.0/xbel-1.0.xhtml" title="http://xbel.sourceforge.net/language/versions/1.0/xbel-1.0.xhtml">http://xbel.sourceforge.net/language/versions/1.0/xbel-1.0.xhtml</a>
See also the discussion at <a href="http://sourceforge.net/mailarchive/forum.php?thread_name=20080618085443.GI25433%40klangraum.plasmasturm.org&amp;forum_name=xbel-specs" title="http://sourceforge.net/mailarchive/forum.php?thread_name=20080618085443.GI25433%40klangraum.plasmasturm.org&amp;forum_name=xbel-specs">http://sourceforge.net/mailarchive/forum.php?thread_name=20080618085443....</a>

The right thing to do is to fix the XBEL DTD and add the icon element, given that it's in the xbel-1.0 spec above.

Much better than breaking compatibility with all existing tools which do expect the icon element to be there; this stuff is already hard to make interoperable (one has to find the icon in the icon theme, or worse, find the favicon file in the kde dirs), but changing the way it's stored in the XML will only make this worse.

- David

On July 1, 2011, 2:37 p.m., Filip Brcic wrote: