Change History (32)

comment:1 follow-up: Changed 9 years ago by alsroot

Replying to tomeu:

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?

what makes sense for SmootTable/TreeView I renamed and added comments

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.

well, in comparing to what Gnome needs, our case is very simple and since new widget is yet ready, proposed using(being very simple) could be useful choice

   2190 +        self._adj = None

"adj" could mean a lot of different things.

renamed to _adjustment

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

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

added _SPARE_ROWS

   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.

not sure how we can avoid gtk circular dependencies coding gtk applications :)

   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.

well, new_cell, for example, could be a class or closure function
so in case of using SmootTable in user code, he should create new class to redefine only these two functions, brrr...

   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?

done

   2259 +    def goto(self, row):

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

done

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

Can we avoid this abbreviation?

done

   2407 +    def _get_head(self):

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

renamed to _get_upper_visible_row

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

What is sender here?

done

   2427 +            class IndexedRow:

Better define all classes always on the module scope level.

done

   2475 +        uplimit = self._adj.upper - page

upper_limit?

done

comment:2 Changed 9 years ago by alsroot

  • Description modified (diff)

comment:3 Changed 9 years ago by alsroot

Replying to tomeu:

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

Can we make these constants private?

removed, in former code, it was also used out of this file

   2692 +        canvas = hippo.Canvas()

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

the reason was using CanvasRoundBox

   2696 +        sel_box = CanvasRoundBox()

Can we avoid that abbr.?

done

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

One more circular reference :(

removed

   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().

done

comment:4 in reply to: ↑ 1 Changed 9 years ago by alsroot

Replying to alsroot:

Replying to tomeu:

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.

well, new_cell, for example, could be a class or closure function
so in case of using SmootTable in user code, he should create new class to redefine only these two functions, brrr...

I changed SmartTable so it has the same ctor arguments as TableView
(SmartTable relies on having do_fill_in() method in passed class)

comment:5 follow-up: Changed 9 years ago by sascha_silbe

I'm a bit puzzled, this ticket is missing all comments from Tomeu. How could that happen? There were no email notifications being sent out for them either, yet alsroot managed to reply to them inside Trac...

comment:6 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by alsroot

Replying to sascha_silbe:

I'm a bit puzzled, this ticket is missing all comments from Tomeu. How could that happen? There were no email notifications being sent out for them either, yet alsroot managed to reply to them inside Trac...

original ticket was #1226

comment:7 Changed 9 years ago by alsroot

  • Summary changed from TreeView widget to TableView widget

comment:8 in reply to: ↑ 6 Changed 9 years ago by sascha_silbe

Replying to alsroot:

Replying to sascha_silbe:

I'm a bit puzzled, this ticket is missing all comments from Tomeu. How could that happen? There were no email notifications being sent out for them either, yet alsroot managed to reply to them inside Trac...

original ticket was #1226

Ah, I see. Would be nice if you could state that and adjust or remove the Trac links as both Trac and I were confused. :)
Since we were having problems with the SPAM filter lately I suspected it to have failed again...

comment:9 follow-up: Changed 9 years ago by alsroot

  • Bug Status changed from Unconfirmed to Needinfo

The question about this widget, how to handle number of row/columns?

Existed code implements scheme with static number of visible columns/rows for widget, so after resizing table widget, number of cells will be preserved and only cells' sizes will be changed.

Another scheme is using static cell sizes and while resizing table widget, number of visible rows/columns will be changed.

And last scheme is leting cells set referable sizes and change number of visible column/rows correspondingly. Not sure if need such complex scheme somewhere in sugar.

comment:10 in reply to: ↑ 9 Changed 9 years ago by alsroot

Replying to alsroot:

The question about this widget, how to handle number of row/columns?

Existed code implements scheme with static number of visible columns/rows for widget, so after resizing table widget, number of cells will be preserved and only cells' sizes will be changed.

Another scheme is using static cell sizes and while resizing table widget, number of visible rows/columns will be changed.

And last scheme is leting cells set referable sizes and change number of visible column/rows correspondingly. Not sure if need such complex scheme somewhere in sugar.

here, cell is Cell widget e.g. in case of Journal, there will Cell class which represent current row and not only one cellrenderer.

comment:11 follow-up: Changed 9 years ago by tomeu

I think we just need a table in which the columns have a preferred fixed width (number of pixels multiple of GRID_CELL) and the extra room is allocated in equal parts between the columns that have the EXPAND flag set. If there isn't enough room, each column is shrinked accordingly.

This should be easily doable in the Gtk+ sizing model, ping me if you find any difficulty.

comment:12 in reply to: ↑ 11 Changed 9 years ago by alsroot

Replying to tomeu:

I think we just need a table in which the columns have a preferred fixed width (number of pixels multiple of GRID_CELL) and the extra room is allocated in equal parts between the columns that have the EXPAND flag set. If there isn't enough room, each column is shrinked accordingly.

This should be easily doable in the Gtk+ sizing model, ping me if you find any difficulty.

just to make clear if I understood it right, For the whole Table widget there is only one Cell class, so EXPAND flag could be for all cells(i.e. Table's property) or for each cell(each Cell instance). In last case, cells from two different rows could be aligned differently e.g in case of thumbs view:

+--------------------------------+
| thumb_1 | thumb_2              |
+--------------------------------+
|     thumb_3    |   thumb_4     |
+--------------------------------+

So, the question is having EXPAND flag for Table or Cell(makes code more complex and could be not so useful) classes.

comment:13 Changed 9 years ago by tomeu

I'm a bit confused, I'm talking about columns but I'm not understanding what you mean by Cell there. If we had a 5x1000 table, would we have 5000 Cell instances?

comment:14 Changed 9 years ago by alsroot

Table is not regular gtk.Container, user won't add subwidgets like to gtk.Box. User just pass Cell class and Table will dynamically create new Cell instance on demand and only for visible cells.

So in case of 5x1000 table, there will only <visible-columns>x<visible-rows> cells. While scrolling table, it will repaint visible cells to represent current "real" cell content.

comment:15 follow-up: Changed 9 years ago by alsroot

At the end Table behaves more like IconView rather then TreeView.
I just kept in mind simple widget but which will satisfy all our needs. For example with current Table we can build Journal with Cell which represent the entirely Journal's row as on Cell class(thus regular gtk widgets instead of cellrenders). Of course we can simulate TreeView by rewriting cellrender in widgets but do we really need such complexity.

comment:16 in reply to: ↑ 15 Changed 9 years ago by tomeu

Replying to alsroot:

At the end Table behaves more like IconView rather then TreeView.
I just kept in mind simple widget but which will satisfy all our needs. For example with current Table we can build Journal with Cell which represent the entirely Journal's row as on Cell class(thus regular gtk widgets instead of cellrenders). Of course we can simulate TreeView by rewriting cellrender in widgets but do we really need such complexity.

Gotcha, then maybe this widget shouldn't be called TableView if it has only one column?

comment:17 Changed 9 years ago by alsroot

Why only one column, one column only for Journal, for Thumbs view it will have several columns(since having only one thumb per row is not so useful). E.g. in case of thumbs, Cell class will contain subwidgets for image, label etc.

comment:18 follow-up: Changed 9 years ago by alsroot

so, what about letting users freedom to choose 1st of 2nd model from http://bugs.sugarlabs.org/ticket/1586#comment:9 ?

comment:19 in reply to: ↑ 18 ; follow-up: Changed 9 years ago by alsroot

Replying to alsroot:

so, what about letting users freedom to choose 1st of 2nd model from http://bugs.sugarlabs.org/ticket/1586#comment:9 ?

e.g.

  • for Journal it will 1x<number-of-visible-rows> and 1st option
  • for thumbs view, it will be min sizes for cell and number of visible columns/rows according to current Table's size

comment:20 Changed 9 years ago by alsroot

or having 1st/2nd options separately for columns-cumber/cell-width and for rows-number/cell-height

comment:21 in reply to: ↑ 19 Changed 9 years ago by tomeu

Replying to alsroot:

Replying to alsroot:

so, what about letting users freedom to choose 1st of 2nd model from http://bugs.sugarlabs.org/ticket/1586#comment:9 ?

e.g.

  • for Journal it will 1x<number-of-visible-rows> and 1st option
  • for thumbs view, it will be min sizes for cell and number of visible columns/rows according to current Table's size

Sounds good, though I will be able to better understand it with two code snippets that set up the view in each case.

comment:22 Changed 9 years ago by alsroot

(examples assume that TableView is model-less class, so we set number of "real" columns/rown manually)

1st option:

table = TableView(cell_class=Cell, cell_numbers=(2,2))
table.column_numbers = 10
table.row_numbers = 10

will produce:

+-------------------+-------------------+
|                   |                   |
|  cell_1(1)        |     cell_2(1)     |
|                   |                   |
+-------------------+-------------------+
|                   |                   |
|  cell_1(2)        |     cell_2(2)     |
|                   |                   |
+-------------------+-------------------+

2st option:

table = TableView(cell_class=Cell, cell_sizes=(1,1))
table.column_numbers = 10
table.row_numbers = 10

will produce:

+---------+---------+---------+---------+
|cell_1(1)|cell_2(1)|cell_3(1)|cell_4(1)|
+---------+---------+---------+---------+
|cell_1(2)|cell_2(2)|cell_3(2)|cell_4(2)|
+---------+---------+---------+---------+
|cell_1(3)|cell_2(3)|cell_3(3)|cell_4(3)|
+---------+---------+---------+---------+
|cell_1(4)|cell_2(4)|cell_3(4)|cell_4(4)|
+---------+---------+---------+---------+

comment:23 Changed 9 years ago by alsroot

  • Component changed from sugar-toolkit to sugar
  • Keywords r? added
  • Owner changed from erikos to tomeu

I've pushed initial refactoring commit to tableview branch
it's not yet fully implemented but it can show how refactoring is going

http://git.sugarlabs.org/projects/sugar/repos/mainline/trees/tableview

comment:24 follow-up: Changed 9 years ago by sascha_silbe

Why don't we put this in sugar-toolkit (sugar.graphics)? There are probably other parts of code (and even activities once the new API is stable enough) that could benefit from such a widget.

comment:25 in reply to: ↑ 24 Changed 9 years ago by alsroot

Replying to sascha_silbe:

Why don't we put this in sugar-toolkit (sugar.graphics)? There are probably other parts of code (and even activities once the new API is stable enough) that could benefit from such a widget.

we need to stabilize API and for me there was another reason - I'm going to reimplement this widget for Vala based toolkit

comment:26 Changed 9 years ago by alsroot

  • Description modified (diff)

For now, it should be full functional journal, list view and thumbs view except DND(I posted a topic for new designers meeting http://wiki.sugarlabs.org/go/Design_Team/0.88_Meeting#Drag_icon) and extended gtk.TreeView features like inplace search(not sure if we need them, at least right now).

I think later coding could be useful only after merging this part.

comment:27 Changed 9 years ago by tomeu

  • Keywords r! added; r? removed

The journal is not starting for me:

  File "/home/tomeu/sugar-jhbuild/install/lib/python2.6/site-packages/jarabe/journal/view.py", line 69, in <lambda>
    VIEW_LIST: lambda: self._view_new(ListView),
  File "/home/tomeu/sugar-jhbuild/install/lib/python2.6/site-packages/jarabe/journal/view.py", line 193, in _view_new
    view = view_class()
TypeError: __init__() takes exactly 2 arguments (1 given)

As an aside, I think this case illustrates the point of using python's dynamic capabilities sparingly: it takes longer to discover what init method the stack trace refers to. In order to know it, I have to trace the code and see which are the possible values of the variable "view_class".

comment:28 Changed 9 years ago by alsroot

  • Keywords r? added; r! removed

I've pushed new commit w/ fix and w/o cell_class

comment:29 Changed 9 years ago by tomeu

  • Milestone changed from 0.88 to 0.90

Moving to 0.90 as per irc

comment:30 Changed 9 years ago by tomeu

  • Keywords r! added; r? removed

Aleksey, is this still in line with your plans for the journal?

comment:31 Changed 9 years ago by alsroot

  • Resolution set to notabug
  • Status changed from new to closed

I'm closing this ticket since it was already implemented in polyol/gui vala library and could be used from there. Not sure if it is useful to support two parallel implementations.

comment:32 Changed 6 years ago by dnarvaez

  • Milestone 0.90 deleted

Milestone 0.90 deleted

Note: See TracTickets for help on using tickets.