Code review for quality control

Registered by Aiman Baharna

Objectives of code review:

- Provide good foundation for future development
- Anticipate plugin development

Blueprint information

Status:
Complete
Approver:
None
Priority:
High
Drafter:
None
Direction:
Approved
Assignee:
None
Definition:
Obsolete
Series goal:
None
Implementation:
Unknown
Milestone target:
None
Completed by
Aiman Baharna

Related branches

Sprints

Whiteboard

Issues that are identified:

(1) Do not store iters in Row objects
 - Row.Iter used by ActionMoveRows.MoveRow()
 - Row.Iter used by ActionDeleteRows.UpdateUndoIters()
 - Row.Iter used by ActionDeleteRows.Undo()
 - Row.Iter used by ActionDeleteRows.Redo()
 - Row.Iter used by Document.UpdateRow()

(2) The use of ActionUndo seems wrong
 - There is no need for this overcomplication.

(3) The use of Document.RecordActions mode seems wrong

(4) The way changes to cells and corresponding event notifications seems wrong
 - Currently all access to cells happens via Row.GetCell
 - Currently all changes to cells *must* happen via Row.SetCell
 - Then Row.SetCell calls Document.CellChanged
 - In RecordActions mode, Document.CellChanged records ActionChangeCell action
 - Ideal solution:
  - It seems more intuitive to let Document handle cell changes not the row
  - There should not be any need to call SetValue or even SetCell
  - Cell properties should automatically notify the document
  - Problem: how to tell whether change should warrant an undo action?
  - Answer:
   - Create explicit Document.SetValue with "record" boolean arg
   - When change is notified from a cell, record=False by default

(5) It seems weird to decouple view columns from data columns
 - The reason for separation needs to be documented some place

(6) UNDECIDED: Whether to have a base column and how to deal with it?
 - Have I decided how the base column is going to be treated?
 - Is an explicit base column necessary, or is it just going to be a free-for-all
 - How to deal with optional settings on the base column?

(7) The program is not plugin-ready:
 - The way things are coded uses too many "type field" instead of virtual functions
 - Need to create some kind of global hashtables for cell types, column types, view columns, etc.
 - Plugins can hook into these by adding new entries to these hash tables, or overriding values, etc

(8) Column traversal on update of a cell needs revamp
 - Need to implement some generic reusable traversal functions, currently they are all local
 - Need to make the checkbox summary traversal more efficient instead of traversing the whole tree
 - Need to prevent infinite recursion when updating cell value triggers another column update
 - Need to prevent undo actions being triggered when updating cell value during column update
 - Currently infinite recursion is prevented with a variable column.Updating

(9) Fix obsolete Gtk method calls which are generating compiler warnings.

(10) Investigate any potential memory leaks relating to TreeIter or TreePath usage
 - May need to defer this goal because it is a big undertaking

(?)

Work Items

This blueprint contains Public information 
Everyone can see this information.

Subscribers

No subscribers.