hi Richard
On Mon, Jan 23, 2012 at 12:00:41AM -0500, Richard Jones wrote:
>
> I would like to wrap up work on the lines, curves, and ellipses branch.
>
> I've renamed the branch, rebased it to the current master, and squashed all
> the commits:
> https://gitorious.org/~optigon/mypaint/optigon-mypaint/commits/rj/lines_curves_ellipses
I'm looking at your code changes now (git diff 1a89aeaa8...a94a9dce8).
Usability has been discussed in the forum already. Thanks to everyone
involved in the testing and feedback cycle there. I only point out some
technical issues in this mail.
First some rambling about git...
I'm always sceptical about merge commits. There is one in your branch,
dd3d458bc78, and it lists a huge number of conflicts. It merges two
branches together that both have a different version of the same commit,
with the result that this commit turns up twice in the history (just run
'gitk' to see it). This was probably the cause for the merge conflict.
Something like that happens when you rebase one branch, and then merge it
into a similar branch that was not rebased the same way. This should be
cleaned up before pushing to master. I would squash the forked history into
a single commit, so that no merge commits remain.
In my experience, git topic-branches work best if you either use only 'git
rebase' (and 'git cherry-pick') or only 'git merge'. If you mix the two,
you have to know what you are doing and double-check the result. I prefer
the 'rebase' approach, but only because I always struggle when reviewing a
merge commit. I'm not strictly against merge commits in master, if they
aren't trivial to rebase and don't look too messy.
... and now comments about the code:
> diff --git a/gui/drawwindow.py b/gui/drawwindow.py
> index a2cdab1..ba0e37a 100644
> --- a/gui/drawwindow.py
> +++ b/gui/drawwindow.py
>
> import colorselectionwindow, historypopup, stategroup, colorpicker,
> windowing, layout, toolbar
> +import linemode
> import dialogs
Unused import. (Not the only one, but let's not add new ones.)
> --- /dev/null
> +++ b/gui/linemode.py
>
> +from lib import layer
Pylint tells me this import isn't used anywhere.
> +for line_list in line_mode_settings_list:
> + l = LineModeSettings()
> + l.cname, l.name, l.constant, l.min, l.default, l.max, l.tooltip =
> line_list
> + l.index = len(line_mode_settings)
> + line_mode_settings.append(l)
> + line_mode_settings_dict[l.cname] = l
> + globals()[l.cname] = l
The globals() assignment can be removed, as far as I see you always use the
dict anyway. (In fact the same is true in brushsettings.py where you got
it from.)
> +class LineMode:
> +
> + ...
> + # Each mode ToggleAction has a corresponding setting
> + def attribute(mode, entry):
> + mode.line_mode = entry[0]
> + mode.stock_id = entry[1]
> + mode.label = entry[2]
> + mode.tooltip = entry[4]
> +
> + for entry in toggle_actions:
> + if entry[0] is "LineModeFreeHand":
> + attribute(self.freehand_mode, entry)
> + if entry[0] is "LineModeStraight":
> + attribute(self.straight_mode, entry)
> + if entry[0] is "LineModeSequence":
> + attribute(self.sequence_mode, entry)
> + if entry[0] is "LineModeEllipse":
> + attribute(self.ellipse_mode, entry)
> + #if entry[0] is "LineModeSmooth":
> + # attribute(self.smooth_mode, entry)
To be honest, this repetitive code looks rather un-pythonic. Just one idea
as input: by using the string "ellipse_mode" instead of "LineModeEllipse",
you could replace all the ifs with a single getattr(self, entry[0]).
> + def current_mode(self):
> + return self.line_mode
Unused method, please remove.
> + def change_line_setting(self, setting, value):
> + if setting == 'entry_pressure':
> + self.entry_pressure = value
> + if setting == 'midpoint_pressure':
> + self.midpoint_pressure = value
> + if setting == 'exit_pressure':
> + self.exit_pressure = value
> + if setting == 'line_head':
> + self.head = value
> + if setting == 'line_tail':
> + self.tail = value
Again, if you choose the strings right, you can replace this whole if-block
with a single setattr(self, setting, value). That would be easier to
maintain; one place less to worry about when something changes.
> + def redraw_line(self):
> + last_line = self.last_line_data
> + if last_line is not None:
> + last_stroke = self.model.layer.get_last_stroke_info()
> + if (last_line[1] == last_stroke):
Please remove the brackets. And maybe "if last_line[1] is last_stroke:"
would be better? I think you don't actually want to compare the content of
the stroke objects (if that was implemented?).
> + if command is "LineModeEllipse":
That is not what you want. Please read again about the difference between
"is" and "==". It happens to work because the Python interpretor reuses the
same object for short strings within the same file, but that's not
guaranteed by the language.
> + self.done = False
> + self.model.split_stroke() # split stroke here
> + self.snapshot = self.model.layer.save_snapshot()
Hm... I'm slightly confused now. I thought you were using the undo stack to
redraw a stroke? So... you use the snapshot only as an optimization to
bypass the full undo/redo operation, right?
> + if self.mode == "LineModeFreeHand":
> + self.mode = mode
Clever :-)
> + if self.mode is "CurveLine2":
> ...
> + if cmd is "CurveLine1":
Again, use "==" here, using "is" is wrong.
> + def get_angle(self, x1, y1, x2, y2):
> + dot = self.dot(x1, y1, x2, y2)
> + if abs(dot) < 1.0:
> + angle = math.acos(dot) * 180/math.pi
> + else:
> + angle = 0.0
> + return angle
There is atan2 for exactly that ('man atan2') which does all special cases
correctly. This is a bit like reinventing the wheel, almost literally :-)
> + def vector_length(self, x, y):
> + length = math.sqrt(x*x + y*y)
> + return length
There is math.hypot() for that, but I agree "length" is more readable.
It's quite unusual that you make all those vector functions part of a class.
None of them operates on 'self', and they are quite generic. I suggest to
convert them to functions.
> + def dot(self, x1, y1, x2, y2):
> + # Dot product for previously normalised vectors ONLY
> + dot_product = x1*x2 + y1*y2
> + return dot_product
Actually, the result is also correct for vectors that are not normalized.
> + def multiply_add(self, x1, y1, x2, y2, d):
> + def multiply(self, x, y, d):
> + def add(self, x1, y1, x2, y2):
> + def difference(self, x1, y1, x2, y2):
An alternative would be to use p=numpy.array([x,y]) as your primary type if
you do lots of vector operations. Then you could just use +, -, * and even
p1.dot(p2). Or maybe make or find a simple 2D vector class? I'm not saying
you must change this, just a comment.
> +# def dynamic_smooth_line(self, x, y):
> +# i = 10
> +# [...30 lines]
Please don't add commented-out code.
The remaining changes look all good to me.
--
Martin Renold
_______________________________________________
Mypaint-discuss mailing list
[email protected]
https://mail.gna.org/listinfo/mypaint-discuss
