[scribus-dev] 0012245: Grouped lines appear thicker

Tue Nov 17 09:02:58 UTC 2015

hey

tl;dr: hey team, it's ok if the HOST guys refactor functions like 
PageItem_Group::DrawObj_Item() so that their purpose is better 
understandable for the reader and bugs are easier to fix?

tl;dr: hey HOST guys, can you write doxygen doc and refactor functions 
that do more than one thing?

> Hi,
> I'm trying to solve the following issue 
> http://bugs.scribus.net/view.php?id=12245 ,
> and found this line in pageitem_group.cpp:193
>
> *p->scale(m_width / groupWidth, m_height / groupHeight);*
>
> What's the purpose of scaling here?! and it's scaling by 1.

this question makes me wonder about the need of refactoring in scribus.

the suggestions by as. to simply remove the

https://github.com/scribusproject/scribus/blob/master/scribus/pageitem_group.cpp#L193

line looks like the right thing to do.

not because it's a wrong line, but because when i look at the 
DrawObj_Item() function, the only purpose -- implicitly  declared -- is 
the general task of drawing the group.
without further notice in the code, i would expect that the method 
simply delegates the drawing to each element in the group.
nothing less, nothing more.
in this context, there should never be a function that modifies the 
scale of a variable which is not local to the function itself.
not without an explanation on why the line is there.


now back to reality.
the line 193 probably has a purpose.

let's find out what the purpose could be!

what as. could do is to do a blame on that line

$ git blame scribus/pageitem_group.cpp -L 190,195
33527943 (Franz Schmid  2014-01-27 19:44:13 +0000 190)  else
33527943 (Franz Schmid  2014-01-27 19:44:13 +0000 191) p->beginLayer(1.0 
- fillTransparency(), fillBlendmode());
69781711 (Jean Ghali    2012-02-10 22:20:30 +0000 192) p->setMaskMode(0);
4aca2e1d (Craig Bradney 2013-01-15 22:03:04 +0000 193) p->scale(m_width 
/ groupWidth, m_height / groupHeight);
69781711 (Jean Ghali    2012-02-10 22:20:30 +0000 194)  for (int em = 0; 
em < groupItemList.count(); ++em)
69781711 (Jean Ghali    2012-02-10 22:20:30 +0000 195)  {

now we know that the line has been last changed by craig in 2013, after 
jean changed the surrounding lines one year early (and we see that franz 
has also added some more lines more recently before those lines.

let's have a look at the commit that introduced that line:

https://github.com/scribusproject/scribus/commit/4aca2e1d#diff-f5de24759cc7fd0558e9d8ab73448db4L190

that was only a general refactoring in pageitem. it does not help us 
finding out the reason for that line.

let's do a git log on that line!


$ git log -L 193,193:scribus/pageitem_group.cpp
commit 4aca2e1d32dcdcb3a417677b29bb13f534717ae3
Author: Craig Bradney <mrb at scribus.info>
Date:   Tue Jan 15 22:03:04 2013 +0000

     Rename PageItem's Width/Height, some more rearranging of pageitem.h

     git-svn-id: svn://scribus.net/trunk/Scribus@18095 
11d20701-8431-0410-a711-e3c959e3b870

diff --git a/scribus/pageitem_group.cpp b/scribus/pageitem_group.cpp
--- a/scribus/pageitem_group.cpp
+++ b/scribus/pageitem_group.cpp
@@ -190,1 +190,1 @@
-       p->scale(Width / groupWidth, Height / groupHeight);
+       p->scale(m_width / groupWidth, m_height / groupHeight);

commit 697817116aa6a550bd648e49a24ec499184e5d67
Author: Jean Ghali <jghali at libertysurf.fr>
Date:   Fri Feb 10 22:20:30 2012 +0000

     some refactoring of graphics related function :
     - improve readability by removing some unnecessary indentation levels
     - split ScPainter::drawVPath() functions
     - remove unused fillPathText() and strokePathText() function from 
ScPainter

     git-svn-id: svn://scribus.net/trunk/Scribus@17288 
11d20701-8431-0410-a711-e3c959e3b870

diff --git a/scribus/pageitem_group.cpp b/scribus/pageitem_group.cpp
--- a/scribus/pageitem_group.cpp
+++ b/scribus/pageitem_group.cpp
@@ -237,0 +190,1 @@
+       p->scale(Width / groupWidth, Height / groupHeight);


ok, the line has been inserted by jean in 2012 while refactoring... we 
have to have a look at his commit, then: no luck, either: the line is older:

112 - p->scale(Width / pat.width, Height / pat.height);

now, if the original author of the line does not step in, you will have 
to go on with this speleological research until you have found out who 
has written that specific line and why!
i stop here, because it's not the main goal of this mail (and i'm no git 
expert), but i hope that the commands i've shown above can help you and 
the other guys to better work with such issues!






and i go back to the main topic: how the hell is somebody supposed to 
understand what that line is doing and how to change it, if in some 
cases it seems to do harm?

what i'd like to suggest you and the team:

PageItem_Group::DrawObj_Item() does urgently need some refactoring!

to make the code more explicit, we don't want to comment each line to 
say what it is doing. we're not programming in assembler...

so, how can we then document what DrawObj_item() is doing?
for sure by writing some doxygen doc for the method.
but also by splitting the 90 lines long method in a few private function 
with telling names, names that automatically tell the reader what is 
going on.


so, my question to the team is: would be it be ok to ask as. to create a 
new private function with the code around

p->scale(m_width / groupWidth, m_height / groupHeight);

so that it's clear what that line does?

in my eyes, in such a case, the programmer should not refactor the whole 
DrawObj_Item() function, just the lines that she fears are offending.
the refactoring should be submitted as a separate patch.
and only after having refactored the code (and in this way having shown 
that she understood what the specific line is doing) she should apply 
the change and submit a patch for it.

of course, the patch for refactoring the existing method, should be 
submitted even if the bug was not there :-)

and, of course, not every line should be in its own function! but in my 
experience 10 lines is somehow the ideal length or a function.


and my proposal to HOST:

- when you touch a function, always try to write a doxygen doc for it 
and make a patch with it (if the team agrees that there is a need for 
it, of course... and the comment should describe what the function does 
and not be obvious: no /** @return returns x */ ABC::getX() {return x;}, 
please :-)

- when you see a function that does multiple things that are not 
summarized in the function name, split the function in private function 
that are specific to the task (always if the team agrees)



voilà, that was a long email.
i hope that it was readable even if i wrote it in one go.
and i hope that this post can be useful to make the scribus code base 
better and better in the future!


have a nice day
a.l.e
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scribus.net/pipermail/scribus-dev/attachments/20151117/69417196/attachment-0001.html>
