[Bugs] #1226 UNSP: Merge thumbs view code
SugarLabs Bugs
bugtracker-noreply at sugarlabs.org
Sun Aug 30 09:33:11 EDT 2009
#1226: Merge thumbs view code
------------------------------------------+---------------------------------
Reporter: alsroot | Owner: tomeu
Type: enhancement | Status: new
Priority: Unspecified by Maintainer | Milestone: Unspecified by Release Team
Component: sugar | Version: 0.85.x
Severity: Unspecified | Resolution:
Keywords: r! | Distribution: Unspecified
Status_field: Unconfirmed |
------------------------------------------+---------------------------------
Comment(by 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.
--
Ticket URL: <http://dev.sugarlabs.org/ticket/1226#comment:6>
Sugar Labs <http://sugarlabs.org/>
Sugar Labs bug tracking system
More information about the Bugs
mailing list