Bug 11287 - Labels
: Labels
Status: RESOLVED FIXED
Product: ReSiStance
General
: unspecified
: All Maemo
: Unspecified enhancement (vote)
: ---
Assigned To: Sergio Villar Senin
: general
:
:
:
:
  Show dependency tree
 
Reported: 2010-09-15 22:36 UTC by Chus
Modified: 2011-06-07 12:10 UTC (History)
2 users (show)

See Also:


Attachments
A patch for labels functionality (29.03 KB, patch)
2010-09-30 23:29 UTC, Chus
Details
This patch is for the interface with labels. It only allows create and remove labels. (12.63 KB, patch)
2010-10-02 12:50 UTC, Chus
Details
This patch is for show the feeds that have a specific label. (11.43 KB, patch)
2010-10-02 14:33 UTC, Chus
Details
Finally, the patch to add and remove feeds to a specific label. (4.52 KB, patch)
2010-10-02 14:56 UTC, Chus
Details
I think that the class FeedsToAddWindow is like FeedsWindow, but this is more simple. Should I leave this class there? (32.92 KB, patch)
2010-10-13 23:23 UTC, Chus
Details
A new patch (updated) (38.37 KB, patch)
2011-02-14 21:56 UTC, Chus
Details


Note

You need to log in before you can comment on or make changes to this bug.


Description Chus (reporter) 2010-09-15 22:36:04 UTC
If ReSiStance allows you to create labels, you can organize the feeds with your
own labels.
Comment 1 Sergio Villar Senin 2010-09-20 11:35:57 UTC
Great idea, actually I was thinking about creating some kind of hierarchy but
indeed, having labels seems more appealing to me. I would really like to see
that :)
Comment 2 Chus (reporter) 2010-09-30 23:29:03 UTC
Created an attachment (id=3110) [details]
A patch for labels functionality
Comment 3 Sergio Villar Senin 2010-10-01 12:35:58 UTC
Wow this is a really big patch. Could you please try to split it in different
patches in order to easen the review process?
Comment 4 Chus (reporter) 2010-10-02 12:50:46 UTC
Created an attachment (id=3116) [details]
This patch is for the interface with labels. It only allows create and remove
labels.
Comment 5 Chus (reporter) 2010-10-02 14:33:40 UTC
Created an attachment (id=3117) [details]
This patch is for show the feeds that have a specific label.
Comment 6 Chus (reporter) 2010-10-02 14:56:45 UTC
Created an attachment (id=3118) [details]
Finally, the patch to add and remove feeds to a specific label.
Comment 7 Sergio Villar Senin 2010-10-03 14:03:01 UTC
(From update of attachment 3116 [details])
Most of the patch looks great, but I have some comments

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff --git a/src/ReSiStance/constants.py b/src/ReSiStance/constants.py
>index cc1fb8a..903b8ee 100644
>--- a/src/ReSiStance/constants.py
>+++ b/src/ReSiStance/constants.py
>@@ -34,6 +34,7 @@ HOME_PATH = os.path.expanduser('~')
> RSS_CONF_FOLDER = os.path.join(HOME_PATH, '.osso/%s' % RSS_COMPACT_NAME)
> RSS_CONF_FILE = os.path.join(RSS_CONF_FOLDER, '%s.conf' % RSS_COMPACT_NAME)
> RSS_DB_FILE = os.path.join(RSS_CONF_FOLDER, '%s' % 'feeds.db')
>+RSS_LABEL_DB_FILE = os.path.join(RSS_CONF_FOLDER, '%s' % 'labels.db')
> 
> DEFAULT_SYSTEM_APP_DIR = os.path.join(sys.prefix,
>                                       'share',
>diff --git a/src/ReSiStance/feedmanager.py b/src/ReSiStance/feedmanager.py
>index 3fa1131..0e0393d 100755
>--- a/src/ReSiStance/feedmanager.py
>+++ b/src/ReSiStance/feedmanager.py
>@@ -90,9 +90,13 @@ class FeedManager(gobject.GObject):
>         super(FeedManager, self).__init__()
> 
>         self.feed_data_list = []
>-
>+        self.labels_list = []
>+        
>     def get_feed_list(self):
>         return self.feed_data_list
>+    
>+    def get_labels_list(self):
>+        return self.labels_list
> 
>     def add_feed(self, url, callback, data=None):
> 
>@@ -221,6 +225,71 @@ class FeedManager(gobject.GObject):
>             callback(key_words, dialog, infolist)
>             gtk.gdk.threads_leave()
> 
>+    def remove_label(self, label):
>+        self.labels_list.remove(label)
>+
>+        # Save to disk
>+        try:
>+            self.save_labels(None)
>+        except IOError:
>+            pass
>+
>+    def create_label(self, label_name, dialog, callback):
>+        if label_name in self.labels_list:
>+            created = False
>+        else:
>+            label_data = {'''label''':{'''name''': label_name}}
>+            self.labels_list.append(label_data)
>+            created = True
>+        callback(label_name, label_data, dialog, created)

This is not correct. Adding labels to a dictionary is not a heavy operation at
all, it's not going to take time so it makes no sense to have a callback or a
dialog as parameters of this function. The only operation that could take time
is the save_labels but that is already an asynchronous operation so you can
call create_label(label_name) safely without a callback.

>+        
>+        # Save to disk
>+        try:
>+            self.save_labels(None)
>+        except IOError:
>+            pass
>+
>+    def load_labels(self, callback=None, data=None):
>+        # TODO: migrate to "with" statement when available
>+        try:
>+            db_file = open(constants.RSS_LABEL_DB_FILE, 'r')
>+        except IOError:
>+            print 'Cannot open', constants.RSS_LABEL_DB_FILE
>+            raise
>+        else:
>+            # Create a new thread to load from disk
>+            loading_thread = Thread(target=self._load_labels_in_thread,
>+                                   args=(db_file, callback, data))
>+            loading_thread.start()
>+
>+    def _load_labels_in_thread(self, db_file, callback, data):
>+        self.labels_list = cPickle.load(db_file)
>+
>+        if callback:
>+            gtk.gdk.threads_enter()
>+            callback(data)
>+            gtk.gdk.threads_leave()
>+
>+    def save_labels(self, callback=None, data=None):
>+        # TODO: migrate to "with" statement when available
>+        try:
>+            db_file = open(constants.RSS_LABEL_DB_FILE, 'w')
>+        except IOError:
>+            print 'Cannot write to', constants.RSS_LABEL_DB_FILE
>+            raise
>+        else:
>+            # Create a new thread to store in disk
>+            saving_labels_thread = Thread(target=self._save_labels_in_thread,
>+                                   args=(db_file, callback, data))
>+            saving_labels_thread.start()
>+
>+    def _save_labels_in_thread(self, db_file, callback, data):
>+        cPickle.dump(self.labels_list, db_file)
>+        if callback:
>+            gtk.gdk.threads_enter()
>+            callback(data)
>+            gtk.gdk.threads_leave()
>+
>     def save(self, callback, data=None):
>         # TODO: migrate to "with" statement when available
>         try:
>diff --git a/src/ReSiStance/gui.py b/src/ReSiStance/gui.py
>index a4dc4fc..c8c4693 100755
>--- a/src/ReSiStance/gui.py
>+++ b/src/ReSiStance/gui.py
>@@ -363,6 +363,7 @@ class FeedsWindow(hildon.StackableWindow):
>     def _on_exit(self, window, event):
>         # Save the feeds status
>         manager.save(self._on_feed_data_saved)
>+        manager.save_labels()
>         settings.save()
> 
>     def _on_feed_data_saved(self, user_data):
>@@ -422,9 +423,19 @@ class FeedsWindow(hildon.StackableWindow):
>         button.connect('clicked', self._find_feed_cb)
>         menu.append(button)
> 
>+        button = hildon.GtkButton(gtk.HILDON_SIZE_FINGER_HEIGHT)
>+        button.set_label(_('Labels'))
>+        button.connect('clicked', self._labels_cb)
>+        menu.append(button)
>+
>         menu.show_all()
>         self.set_app_menu(menu)
> 
>+    def _labels_cb(self, button):
>+        news_window = LabelsWindow()
>+        hildon.Program.get_instance().add_window(news_window)
>+        news_window.show()
>+
>     def _new_feed_cb(self, button):
>         dialog = NewFeedDialog(self)
>         dialog.connect('response', self._new_feed_response_cb)
>@@ -1308,3 +1319,216 @@ class FindWindow(hildon.StackableWindow):
> 
>     def _restore_normal_mode(self, button):
>         self.destroy()
>+
>+class LabelsView(hildon.GtkTreeView):
>+
>+    LABEL_DATA_COLUMN = 0
>+    LABEL_NAME_COLUMN = 1
>+
>+    def __init__(self, ui_mode):
>+        super(LabelsView, self).__init__(ui_mode)
>+
>+        # Add columns
>+        text_renderer = gtk.CellRendererText()
>+        text_renderer.set_property('ellipsize', ELLIPSIZE_END)
>+        column = gtk.TreeViewColumn('Name', text_renderer, markup=1)
>+        column.set_expand(True)
>+        self.append_column(column)
>+
>+class LabelsWindow(hildon.StackableWindow):
>+
>+    def __init__(self):
>+        super(LabelsWindow, self).__init__()
>+
>+        # Load Labels (could be none)
>+        try:
>+            manager.load_labels(self._labels_loaded)
>+        except IOError:
>+            pass
>+
>+        self.view = LabelsView(gtk.HILDON_UI_MODE_NORMAL)
>+        self.view.connect ("row-activated", self._on_label_activated)
>+        self.view.show()
>+
>+        self.set_title('Labels')
>+        self._create_menu()
>+
>+        # Edit toolbar
>+        self.edit_toolbar_button_handler = 0
>+        self.edit_toolbar = hildon.EditToolbar()
>+        self.edit_toolbar.connect('arrow-clicked', self._restore_normal_mode)
>+
>+        self.set_edit_toolbar(self.edit_toolbar)
>+
>+        self.view.set_model (gtk.ListStore(gobject.TYPE_PYOBJECT, str))
>+
>+        self.view.show()
>+
>+        self.align = gtk.Alignment(0, 0, 1, 1)
>+        self.align.set_padding(4, 0 , 16, 16)
>+
>+        pannable = hildon.PannableArea()
>+        pannable.add (self.view)
>+        pannable.set_size_request_policy(hildon.SIZE_REQUEST_CHILDREN)
>+        pannable.show()
>+
>+        self.align.add(pannable)
>+        self.add(self.align)
>+        self.align.show()
>+
>+    def _labels_loaded(self, user_data):
>+        try:
>+            labels = manager.get_labels_list()
>+        except IOError:
>+            print 'cannot open', constants.RSS_LABEL_DB_FILE
>+        else:
>+            store = self.view.get_model()
>+
>+            for label in labels:
>+                feed_iter = store.append()
>+
>+                store.set(feed_iter, self.view.LABEL_NAME_COLUMN, gobject.markup_escape_text(label['label']['name']),
>+                      self.view.LABEL_DATA_COLUMN, label)
>+                
>+        if len(labels) == 0:
>+            self.remove_button.hide()
>+
>+    def _create_menu(self):
>+        menu = hildon.AppMenu()
>+
>+        button = hildon.GtkButton(gtk.HILDON_SIZE_FINGER_HEIGHT)
>+        button.set_label(_('New Label'))
>+        button.connect('clicked', self._create_label_cb)
>+        menu.append(button)
>+
>+        button = hildon.GtkButton(gtk.HILDON_SIZE_FINGER_HEIGHT)
>+        button.set_label(_('Remove Label'))
>+        button.connect('clicked', self._remove_label_cb)
>+        self.remove_button = button
>+        menu.append(button)
>+
>+        menu.show_all()
>+        self.set_app_menu(menu)        
>+        
>+    def _create_label_cb(self, button):
>+        dialog = CreateLabelsDialog(self)
>+        dialog.connect('response', self._create_label_response_cb)
>+        dialog.show_all()
>+    
>+    def _create_label_response_cb(self, dialog, response):
>+        if response == gtk.RESPONSE_ACCEPT:
>+            label_name = dialog.entry.get_text()
>+
>+            # Quickly show feedback to user
>+            hildon.hildon_gtk_window_set_progress_indicator(dialog, True)
>+
>+            manager.create_label(label_name, dialog, self._label_created_cb)
>+            dialog.set_response_sensitive(gtk.RESPONSE_ACCEPT, False)

If you see my comment above about create_label you'll easily realize that you
don't need the label_created_cb anymore. Just move the code of the callback
here.

>+    
>+    def _label_created_cb(self, label_name, label_data, dialog, created):
>+        dialog.set_response_sensitive(gtk.RESPONSE_ACCEPT, True)
>+        hildon.hildon_gtk_window_set_progress_indicator(dialog, False)
>+        
>+        if created:
>+            store = self.view.get_model()
>+            feed_iter = store.append()
>+            # Update model
>+            store.set(feed_iter, self.view.LABEL_NAME_COLUMN, gobject.markup_escape_text(label_name),
>+                      self.view.LABEL_DATA_COLUMN, label_data)
>+            
>+            self.remove_button.show()
>+            
>+            hildon.hildon_banner_show_information(self, '', _('Label created.'))
>+        else:
>+            hildon.hildon_banner_show_information(self, '', _('Label not created. Already exists.'))
>+
>+        dialog.destroy()    
>+    
>+    def _remove_label_cb(self, button):
>+        # tree view edit mode
>+        hildon.hildon_gtk_tree_view_set_ui_mode(self.view, gtk.HILDON_UI_MODE_EDIT)
>+        self.view.get_selection().set_mode(gtk.SELECTION_MULTIPLE)
>+        self.view.get_selection().unselect_all()
>+
>+        self.edit_toolbar.set_label(_('Select Labels to remove'))
>+        self.edit_toolbar.set_button_label(_('Remove'))
>+        if self.edit_toolbar.handler_is_connected (self.edit_toolbar_button_handler):
>+            self.edit_toolbar.disconnect(self.edit_toolbar_button_handler)
>+        self.edit_toolbar_button_handler = \
>+            self.edit_toolbar.connect('button-clicked', self._remove_label_button_clicked_cb)
>+
>+        self.set_edit_toolbar(self.edit_toolbar)
>+        self.edit_toolbar.show()
>+        self.fullscreen()
>+
>+    def _remove_label_button_clicked_cb(self, button):
>+        selection = self.view.get_selection()
>+        selected_rows = selection.get_selected_rows()
>+        model, paths = selected_rows
>+        if not paths:
>+            hildon.hildon_banner_show_information(self, '', _('Please select at least one Label'))
>+            return
>+
>+        removed = []
>+        for path in paths:
>+            try:
>+                manager.remove_label(model[path][LabelsView.LABEL_DATA_COLUMN])
>+            except IOError:
>+                print 'Could not remove', constants.RSS_DB_FILE
>+                hildon.hildon_banner_show_information(self, '', _('File error while removing Label'))
>+            else:
>+                removed.append(gtk.TreeRowReference(model, path))
>+
>+        for reference in removed:
>+            model.remove(model.get_iter(reference.get_path()))
>+
>+        try:
>+            labels = manager.get_labels_list()
>+        except IOError:
>+            print 'cannot open', constants.RSS_LABEL_DB_FILE
>+        else:
>+            if len(labels) == 0:
>+                self.remove_button.hide()
>+
>+        # restore normal mode
>+        self._restore_normal_mode(button)
>+
>+    def _restore_normal_mode(self, button):
>+        # tree view normal mode
>+        hildon.hildon_gtk_tree_view_set_ui_mode(self.view, gtk.HILDON_UI_MODE_NORMAL)
>+        self.view.get_selection().unselect_all()
>+
>+        self.edit_toolbar.hide()
>+        self.unfullscreen()
>+
>+    def _on_label_activated(self, treeview, path, column):
>+        label_iter = self.view.get_model().get_iter(path)
>+        label_data = self.view.get_model().get_value(label_iter, self.view.LABEL_DATA_COLUMN)
>+        
>+        news_window = LabelFeedsWindow(label_data)
>+        hildon.Program.get_instance().add_window(news_window)
>+        news_window.show()

I'll talk about the LabelFeedsWindow in the other patch

>+
>+class CreateLabelsDialog(gtk.Dialog):
>+    def __init__(self, parent):
>+        super(CreateLabelsDialog, self).__init__(_('Create Labels'), parent,
>+                                            gtk.DIALOG_MODAL | gtk.DIALOG_DESTROY_WITH_PARENT,
>+                                            (_('Create'), gtk.RESPONSE_ACCEPT))
>+
>+
>+        self.entry = hildon.Entry(gtk.HILDON_SIZE_FINGER_HEIGHT)
>+        self.entry.set_input_mode(gtk.HILDON_GTK_INPUT_MODE_FULL)
>+        self.entry.connect('changed', self._entry_changed_cb)
>+
>+        caption = gtk.HBox(False, 16)
>+        caption.pack_start(gtk.Label(_('Label Name') + ':'), False, False)
>+        caption.pack_start(self.entry)
>+        self.vbox.add(caption)
>+
>+        self.set_response_sensitive(gtk.RESPONSE_ACCEPT, False)
>+
>+    def _entry_changed_cb(self, entry):
>+        if len(entry.get_text()) > 0:
>+            self.set_response_sensitive(gtk.RESPONSE_ACCEPT, True)
>+        else:
>+            self.set_response_sensitive(gtk.RESPONSE_ACCEPT, False)
Comment 8 Sergio Villar Senin 2010-10-03 14:23:57 UTC
(From update of attachment 3117 [details])
><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff --git a/src/ReSiStance/feedmanager.py b/src/ReSiStance/feedmanager.py
>index 0e0393d..23814c8 100755
>--- a/src/ReSiStance/feedmanager.py
>+++ b/src/ReSiStance/feedmanager.py
>@@ -388,6 +388,10 @@ class FeedManager(gobject.GObject):
>             return
> 
>         new_feed_data = ReSiStanceFeedDict(feedparser.parse(url))
>+        
>+        if not 'labels' in new_feed_data:
>+            new_feed_data['labels'] = []
>+        

You're doing this only for new feeds, but what about feeds that we already have
and do not have the 'labels' key? You need to add it for all of them or always
check the presence of 'labels' before accessing it.

>         self.feed_data_list.append(new_feed_data)
> 
>         if 'link' in new_feed_data.feed:
>diff --git a/src/ReSiStance/gui.py b/src/ReSiStance/gui.py
>index c8c4693..9c71be3 100755
>--- a/src/ReSiStance/gui.py
>+++ b/src/ReSiStance/gui.py
>@@ -1532,3 +1532,261 @@ class CreateLabelsDialog(gtk.Dialog):
>             self.set_response_sensitive(gtk.RESPONSE_ACCEPT, True)
>         else:
>             self.set_response_sensitive(gtk.RESPONSE_ACCEPT, False)

>+class LabelFeedsWindow(hildon.StackableWindow):
>+
>+    def __init__(self, label_data):
>+        super(LabelFeedsWindow, self).__init__()

I have a lot of doubts about LabelFeedsWindow (and LabelFeedView). Basically is
the same window as FeedsWindow but with different options in the menu. My
intuition is that we should have one class, as use an optional argument that
will instruct __init__ to create a "normal" FeedsWindow or a FeedsWindow for
labels. That argument will be most likely be used by create_menu for example to
only add the proper options.
Comment 9 Sergio Villar Senin 2010-10-03 14:28:46 UTC
(From update of attachment 3118 [details])
><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff --git a/src/ReSiStance/feedmanager.py b/src/ReSiStance/feedmanager.py
>index 23814c8..f7b034b 100755
>--- a/src/ReSiStance/feedmanager.py
>+++ b/src/ReSiStance/feedmanager.py
>@@ -290,6 +290,40 @@ class FeedManager(gobject.GObject):
>             callback(data)
>             gtk.gdk.threads_leave()
> 
>+    def add_feed_label(self, feed, label, callback):
>+        # Create a new thread to update from network
>+        adding_thread = Thread(target=self._add_feed_label_in_thread,
>+                               args=(feed, label, callback))
>+        adding_thread.start()
>+
>+    def _add_feed_label_in_thread(self, feed_data, label, callback):
>+        feed_data['labels'].append(label['label']['name'])

I guess you should either add the 'labels' key to all feeds that don't have it
when loading them or always check with has_key() that you have it before trying
to use it.
Comment 10 Chus (reporter) 2010-10-13 23:23:38 UTC
Created an attachment (id=3125) [details]
I think that the class FeedsToAddWindow is like FeedsWindow, but this is more
simple. Should I leave this class there?
Comment 11 Sergio Villar Senin 2010-10-14 21:46:34 UTC
(In reply to comment #10)
> Created an attachment (id=3125) [details] [details]
> I think that the class FeedsToAddWindow is like FeedsWindow, but this is more
> simple. Should I leave this class there?
> 

I saw how you refactored the FeedsWindow and the LabelsWindow, great job! I
don't want to be picky but do not use "kind", I prefer something like
window_type for example. It seems that class is doing a lot of things and that
we should remove some of them from there in the future. For example all the
i18n stuff and some other things shouldn't be in the window.

BTW don't you think that gui.py is growing too much and that we should consider
for example moving the feedswindow out? I'd really like to hear your opinion
about that.

Regarding your question, if you think it could take a lot of work to refactor
it (or complicate too much the FeedsWindow class), I'd leave it as it is.
Comment 12 Chus (reporter) 2010-10-17 20:56:45 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=3125) [details] [details] [details]
> > I think that the class FeedsToAddWindow is like FeedsWindow, but this is more
> > simple. Should I leave this class there?
> > 
> 
> I saw how you refactored the FeedsWindow and the LabelsWindow, great job! I
> don't want to be picky but do not use "kind", I prefer something like
> window_type for example. It seems that class is doing a lot of things and that
> we should remove some of them from there in the future. For example all the
> i18n stuff and some other things shouldn't be in the window.
> 
> BTW don't you think that gui.py is growing too much and that we should consider
> for example moving the feedswindow out? I'd really like to hear your opinion
> about that.

Maybe you're right, gui.py is large. I'll put feedswindow in other file, won't
I?

> 
> Regarding your question, if you think it could take a lot of work to refactor
> it (or complicate too much the FeedsWindow class), I'd leave it as it is.
>
Comment 13 Sergio Villar Senin 2010-10-18 11:06:51 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Created an attachment (id=3125) [details] [details] [details] [details]
> > > I think that the class FeedsToAddWindow is like FeedsWindow, but this is more
> > > simple. Should I leave this class there?
> > > 
> > 
> > I saw how you refactored the FeedsWindow and the LabelsWindow, great job! I
> > don't want to be picky but do not use "kind", I prefer something like
> > window_type for example. It seems that class is doing a lot of things and that
> > we should remove some of them from there in the future. For example all the
> > i18n stuff and some other things shouldn't be in the window.
> > 
> > BTW don't you think that gui.py is growing too much and that we should consider
> > for example moving the feedswindow out? I'd really like to hear your opinion
> > about that.
> 
> Maybe you're right, gui.py is large. I'll put feedswindow in other file, won't
> I?

After thinking about it twice I think it'd be better to do it after landing the
big patches for google reader, labels and podcasting.
Comment 14 Chus (reporter) 2011-02-14 21:56:20 UTC
Created an attachment (id=3291) [details]
A new patch (updated)
Comment 15 Sergio Villar Senin 2011-06-07 12:10:01 UTC
I'm closing this as labels support was integrated in 0.9.2 release.