Implement thumbs view plugin

[alsroot]

 http://wiki.sugarlabs.org/go/Features/Thumbs_View_in_Journal
 http://git.sugarlabs.org/projects/sugar/repos/thumbs

[alsroot]

I can see odd bug with dublication Journal entries,
and trying to figure out if it was landed by new code or not

[tomeu]

Replying to alsroot:

I can see odd bug with dublication Journal entries,
and trying to figure out if it was landed by new code or not

So is there any known bug related to this patch?

[alsroot]

Replying to tomeu:

Replying to alsroot:

I can see odd bug with dublication Journal entries,
and trying to figure out if it was landed by new code or not

So is there any known bug related to this patch?

I re-created ds environment and can't reproduce duplication issue
(int fact I'm not sure did I get duplicated Journal entries after implementing thumbs view or they existed before)

[tomeu]

      1 diff --git a/examples/jarabe b/examples/jarabe
      2 new file mode 120000
      3 index 0000000..2ed1cfb
      4 --- /dev/null
      5 +++ b/examples/jarabe
      6 @@ -0,0 +1 @@
      7 +../src/jarabe
      8 \ No newline at end of file

Pushed by mistake?

    364 +        self.columns_by_name = {}
    365 +        self.columns_by_num = {}
    366 +        self.columns_types = {}

I guess these don't need to be public any more?

    379 +        self._n_columns = max(self.columns_types.keys()) + 1

I think using len() would be clearer, otherwise the reader thinks that something clever is going on.

    408 +    def set_view(self, view, force=False):

I find it really unfortunate that the model needs to reference the view, is there really no way to avoid the circular reference? The point of having a view and a model is precisely that the model doesn't know anything about the view.

    467 +    def get_row(self, pos, frame=None):

Why adding this public method to a TreeModel? Cannot use the existing TreeModel API? "pos" could mean several things, please use position instead.

    469 +            return False

If this method is supposed to return a Row, wouldn't that be None instead of False?

    494 +            row = Row(self.columns_by_name, self.on_calc_value, (offset, ),

Uff, by how you pass a reference to that method to Row, looks like Row and Model are still very tightly coupled, couldn't be the same class? Or could we split functionality in some other way?

    533 +    def in_frame(self, offset):

Do we need to expose the frame concept? Couldn't be something internal to the model? As far as I understand it, it's a mechanism for implementing readahead and cache, thus an optimization.

    557 +            return row is not None and row != False and row[column]

s/row != False/row ?

   1399 -            obj = self._dict[uid]
   1400 +            obj = self._dict.get(uid)
   1401 +            if obj is None:
   1402 +                continue

Why this? In which case it's not an error to request that an entry is removed but that entry isn't in the cache? In the older code we were able to catch these errors, with this change, these errors will be silently ignored and thus bugs will be harder to catch.

   1506 -def find(query, page_size):
   1507 +
   1508 +def find(query_, page_size):

Why do we need this renaming?

   1549 +                error_handler=lambda e: reply_cb(None))

Doesn't this mean that nobody reports the actual error because e is lost there?

   1660 -        self._list_view = ChooserListView()
   1661 -        self._list_view.connect('entry-activated', self.__entry_activated_cb)
   1662 -        vbox.pack_start(self._list_view)
   1663 -        self._list_view.show()
   1664 +        self._object_view = ObjectView()
   1665 +        self._object_view.props.hover_selection = True
   1666 +        self._object_view.connect('entry-activated', self.__entry_activated_cb)
   1667 +        vbox.pack_start(self._object_view)
   1668 +        self._object_view.show()

I personally preferred how it was done before. By having a class for the chooser, another for the list view and a base class with the common code, we split responsibilities in an obvious way. By having everything in a single class and using a flag we make the code more fragile and hard to understand because a single class needs to do more stuff.

   1788 +                    int(row[Source.FIELD_TIMESTAMP]) or 0)

Has "or 0" any effect here? AFAIK, int() will always return an int.

   1786 +        if column == Source.FIELD_MODIFY_TIME:

Any reason to not use the existing terminology? When we have versions, entries will be immutable, so "modify time" won't be appropriate.

   1797 +    def discard_thumb(self, row):
...
   1802 +    def fetch_thumb(self, row):

Does this two need to be public? Cannot be done internally to the model?

   1817 +    def __idle_cb(self):
   1818 +        while len(self._fetch_queue):

A common idiom for what you are doing is to use the pop() method on the queue.

   1820 +            if self.in_frame(row.path[0]):
   1821 +                self.source.get_object(row, self.__get_object_cb)
   1822 +                break
   1823 +            del self._fetch_queue[0]

Better use else: in these cases.

   1883 +VIEW_TYPES = [ListView, ThumbsView]

If we are going to have only two views here, I think it would be better not to make the code generic, because we don't remove much redundant code but make things much harder to read. If you anticipate having a third one in the near future, then I'm ok with this.

   1917 +            view.modify_base(gtk.STATE_NORMAL,
   1918 +                    style.COLOR_WHITE.get_gdk_color())

Can we have this in the constructor of each class?

   2179 +class SmoothTable(gtk.Container):

Are you really using any Container functionality? If not, may be better to inherit from Widget instead because this widget isn't behaving like a Container is expected to.

I'm not very happy with the design of SmoothTable and LazyModel. The other classes need to know about bin_rows, frames, cursors, models, sources, and I cannot figure out what more than half of those are. This is raising significantly the bar to hack on the journal without a high risk of adding regressions. In the existing lazy model, we encapsulate all the cache and readahead business inside model.py and it is not exposed to user classes, why do we must drop the benefits of such separation of concerns in order to get a thumbs view?

Doing a good class design for this is hard work, that's why gtk.TreeView sucks so much presently. There's an ongoing discussion about how to improve the TreeView class design in Gtk+ right now, see Kristian Rietveld's and Philip Van Hoof's blogs about it.

   2190 +        self._adj = None

"adj" could mean a lot of different things.

   2202 +        for i_ in range(rows + 2):

'2' is a magical number, can we use a constant?

   2207 +                cell.set_parent(self)
   2208 +                cell.size_allocate(gtk.gdk.Rectangle(-1, -1))
   2209 +                row.append(cell)

One more circular dependency :( We should be spending more time designing the class model and less time coding and debugging.

   2186 +    def __init__(self, rows, columns, new_cell, fill_in):
...
   2205 +                cell = new_cell()

How we use to do this is by having an abstract method that concrete subclasses can implement. In python is very easy and powerful to pass around references to methods but gets messy very quickly because it's hard to find where is defined the method that will be called. Every time you are tempted to pass a method reference, consider using instead an abstract method or a signal.

   2214 +    def get_columns(self):
...
   2219 +    def get_rows(self):

I would expect that a method named like that would return a collection of all the rows or columns. What about calling them get_row_count and get_column_count?

   2259 +    def goto(self, row):

What about calling it scroll_to_row similarly to what gtk.TreeView already has?

   2399 +            callocation = gtk.gdk.Rectangle(cell_x, cell_y)

Can we avoid this abbreviation?

   2407 +    def _get_head(self):

Cannot get what this method does, can we have a more descriptive name for it?

   2412 +    def __adjustment_value_changed_cb(self, sender=None):

What is sender here?

   2427 +            class IndexedRow:

Better define all classes always on the module scope level.

   2475 +        uplimit = self._adj.upper - page

upper_limit?

   2564 +                   'thumb': (FIELD_THUMB, cairo.ImageSurface)}

It's a bit surprising that a model class depends on Cairo. What about the model providing the PNG image as a string and the view rendering it to a cairo surface?

   2506 +++ b/src/jarabe/journal/source.py

What's the point of having this class? If it's just to hold the column definitions, shouldn't be called ModelHeader or something similar? Or perhaps can be just constants at the module level?

   2625 +COLOR_BACKGROUND = style.COLOR_WHITE
   2626 +COLOR_SELECTED = style.COLOR_TEXT_FIELD_GREY

Can we make these constants private?

   2692 +        canvas = hippo.Canvas()

Do we really need to use HippoCanvas? I think we should be reducing its usage, not increasing it.

   2696 +        sel_box = CanvasRoundBox()

Can we avoid that abbr.?

   2700 +        cell = cell_class()
   2701 +        cell.table = self

One more circular reference :(

   2708 +        canvas.table_view_cell_sel_box = sel_box
   2709 +        canvas.table_view_cell = cell

All class attributes should be set in the constructor, so in this case we would need a subclass of hippo.Canvas().

   2813 +class ThumbsCell(TableCell, hippo.CanvasBox):

Can we avoid multiple inheritance? In some rare occasions is the most natural design, but I don't think this is the case here.

   2941 +        self.table.emit('detail-clicked', self.row[Source.FIELD_UID])

Where the table attribute comes from? If to find out I have to grep the sources for "table" and scan every occurrence, we are doing something wrong. What we use to do is to define a signal in TableCell that Table listens for and then emits detail-clicked itself. A bit more verbose (4 lines more?), but you avoid having those circular references.

   2954 +class ActivityCanvas(object):
...
   2958 +        self.connect_after('button-release-event',

The type 'object' doesn't have a connect_after method. I see how this class is used below, but this is very hard to read and understand. Please try to find a more simple class design. This also applies to other classes in this module.

   3030 +    def __on_leave_cb(self, button):

This doesn't seem to be a real callback.

---

I'm a bit unsure now about landing this. Even if all comments are addressed (some point to problems that are not easy to solve without considerable thinking and effort) this is going to have a big impact on the journal stability.

The patch is 3073 lines, while the existing journal total code base is 3749. After the patch is applied, it jumps to 5234.

Instead of using existing gtk widgets, it creates a new one from scratch for the thumbs view, with custom scrolling logic. The model is complex, with several connections between the model and view, even circular connections. It also uses hippo, which has no keyboard navigation nor accessibility support.

I was positive about landing it when we talked last week based on its performance, memory usage and some skimming of the patch, but I really expected that it would be substantially less invasive.

I also don't know why we have these so big contributions past the deadlines. If we have a schedule is to make it possible to work efficiently as a team. Not only as the Sugar development team but also with distributions, deployers, testers, bug triagers, the marketing team, etc.

Our work together with those other people depends on our capability of sticking to the schedule, that's why the schedule is announced in advanced and comments are requested at that moment.

I'm really worried now because our git HEAD is a big mess now, full of bugs, several of them very serious that impede people test Sugar. We should have been very focused on fixing bugs since a week ago and instead we are discussing these so invasive changes.

Also, I haven't heard much from users about how this feature is important for them, I would like that the users that do care about it explain it in the mailing list.

[alsroot]

Thumb view will be formed in view plugin
after implementing  http://wiki.sugarlabs.org/go/Features/Unified_Browser_for_Objects

[tomeu]

Any update on this? Have been reports from the fields about being able to browse the journal with thumbnails (from the same journal activity). I'm a bit confused about the component being ActivityTeam.

[alsroot]

I guess it is better to implement this feature in activity(Library) at first to make it round and also plugins in journal doesn't seem to be obvious.