Thank you very much maxy for the detailed report. I had a chance to fix the merge conflicts and commited the new branch as rj/linemode. The icons can be added by merging with Deevad's icon branch. None of the code changes have been attempted yet. I was not able to apply savageorange's patch after fixing the merge conflicts. I'll continue as I can.
Thanks again for your support :) On Tue, Feb 14, 2012 at 2:00 PM, Martin Renold <[email protected]> wrote: > 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
