On Tue, 23 Feb 2010 09:05:11 +0100, Thomas Bruederli roundcube@gmail.com wrote:
On Sun, Jan 17, 2010 at 08:41, Roland Liebl roland@roland-liebl.de
wrote:
Hi all,
Hi Roland
the default RSS of Roundcube does not play nice with IE. Also HTTP Authentication is missing. I've made a plugin for RSS support. Please test it! Any feedback is welcome.
I finally had a look at your plugin and it (almost) works for me. Over all it looks way too complicated to me. Here are some suggestions:
- Don't use imap_mime_header_decode() because the IMAP extension of
PHP might not be available. Use $IMAP->decode_header() instead.
done
- You have added loads of code for charset detection and conversion.
rcube_imap returns values already encoded in UTF-8 so you shouldn't need to care about that. Just make sure that whithin the foreach loop you set $IMAP->set_charset($item->charset ? $item->charset : $rcmail->config->get('default_charset', 'UTF-8'));
done
- Your plugin adds an RSS button somewhere to the page. Why not using
the default way and adding a <link rel="alternate" ...> and let the browser do the rest? In this case you don't need to worry about skins and different styles. This could easily be achieved with $rcmail->output->add_header(html::tag('link', array('rel' => 'alternate', 'type' => 'application/rss+xml', 'href' => './?_action=plugin.rss', 'title' => "RSS Feed")));
Perhaps you misunderstood this part of the code. A click on the button will popup the RSS url which the user has to enter into his rss reader.
- With the recent changes in the svn trunk version you also need to
register the plugin in task "login" public $task = "mail|login";
done: public $task = "mail|login|logout";
- Personally I don't want to see an extra checkbox on the login
screen. Maybe this could be make configurable.
done
- Adding $item->from to the subject seems not necessary since most RSS
readers also display the autor-field where we have the from address.
done (IE does ... I have to check with firefox)
- Why adding numbers to the subjects? These numbers can change if you
delete a message or if new ones arrive and then my RSS reader gets confused. I'd prefer only to see the subjects.
removed
- And why do you encode all chars with hex-entities? IMO
htmlspecialchars() should do the trick.
There are some special chars which are not handled correct this way. But I will think about to simplify it.
It's definitely a good replacement for the crappy RSS output Roundcube currently offers. Maybe you should rename it to rss_feed for better understanding. However, feel free to add your plugin to our repository. I'll definitely remove the rss step from the core codebase of Roundcube.
Regards, Thomas
Thanks for the review.
--- 8< --- detachments --- 8< --- The following attachments have been detached and are available for viewing. http://detached.gigo.com/rc/1Q/Si5pYWTD/vcard.vcf Only click these links if you trust the sender, as well as this message. --- 8< --- detachments --- 8< ---
List info: http://lists.roundcube.net/dev/