<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
hey<br>
<br>
tl;dr: hey team, it's ok if the HOST guys refactor functions like <span
class="pl-en">PageItem_Group::DrawObj_Item</span>() so that their
purpose is better understandable for the reader and bugs are easier
to fix?<br>
<br>
tl;dr: hey HOST guys, can you write doxygen doc and refactor
functions that do more than one thing?<br>
<br>
<blockquote
cite="mid:CAPKqcuSggZ+nwKW=FeL8Ocd+wbHGqhEwYR6phORXzENNxiBH7w@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>
<div>
<div>
<div>
<div>Hi,<br>
</div>
I'm trying to solve the following issue <a
moz-do-not-send="true"
href="http://bugs.scribus.net/view.php?id=12245"><a class="moz-txt-link-freetext" href="http://bugs.scribus.net/view.php?id=12245">http://bugs.scribus.net/view.php?id=12245</a></a>
,<br>
</div>
and found this line in pageitem_group.cpp:193<br>
<br>
<b>p->scale(m_width / groupWidth, m_height /
groupHeight);</b><br>
<br>
</div>
What's the purpose of scaling here?! and it's scaling by 1.<br>
</div>
</div>
</div>
</blockquote>
<br>
this question makes me wonder about the need of refactoring in
scribus.<br>
<br>
the suggestions by as. to simply remove the<br>
<br>
<a class="moz-txt-link-freetext" href="https://github.com/scribusproject/scribus/blob/master/scribus/pageitem_group.cpp#L193">https://github.com/scribusproject/scribus/blob/master/scribus/pageitem_group.cpp#L193</a><br>
<br>
line looks like the right thing to do.<br>
<br>
not because it's a wrong line, but because when i look at the <span
class="pl-en">DrawObj_Item</span>(<span class="pl-c"></span>)
function, the only purpose -- implicitly declared -- is the general
task of drawing the group.<br>
without further notice in the code, i would expect that the method
simply delegates the drawing to each element in the group.<br>
nothing less, nothing more.<br>
in this context, there should never be a function that modifies the
scale of a variable which is not local to the function itself.<br>
not without an explanation on why the line is there.<br>
<br>
<br>
now back to reality.<br>
the line 193 probably has a purpose.<br>
<br>
let's find out what the purpose could be!<br>
<br>
what as. could do is to do a blame on that line<br>
<br>
$ git blame scribus/pageitem_group.cpp -L 190,195<br>
33527943 (Franz Schmid 2014-01-27 19:44:13 +0000 190) else<br>
33527943 (Franz Schmid 2014-01-27 19:44:13 +0000 191)
p->beginLayer(1.0 - fillTransparency(), fillBlendmode());<br>
69781711 (Jean Ghali 2012-02-10 22:20:30 +0000 192)
p->setMaskMode(0);<br>
4aca2e1d (Craig Bradney 2013-01-15 22:03:04 +0000 193)
p->scale(m_width / groupWidth, m_height / groupHeight);<br>
69781711 (Jean Ghali 2012-02-10 22:20:30 +0000 194) for (int em
= 0; em < groupItemList.count(); ++em)<br>
69781711 (Jean Ghali 2012-02-10 22:20:30 +0000 195) {<br>
<br>
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.<br>
<br>
let's have a look at the commit that introduced that line:<br>
<br>
<a class="moz-txt-link-freetext" href="https://github.com/scribusproject/scribus/commit/4aca2e1d#diff-f5de24759cc7fd0558e9d8ab73448db4L190">https://github.com/scribusproject/scribus/commit/4aca2e1d#diff-f5de24759cc7fd0558e9d8ab73448db4L190</a><br>
<br>
that was only a general refactoring in pageitem. it does not help us
finding out the reason for that line.<br>
<br>
let's do a git log on that line!<br>
<br>
<br>
$ git log -L 193,193:scribus/pageitem_group.cpp<br>
commit 4aca2e1d32dcdcb3a417677b29bb13f534717ae3<br>
Author: Craig Bradney <a class="moz-txt-link-rfc2396E" href="mailto:mrb@scribus.info"><mrb@scribus.info></a><br>
Date: Tue Jan 15 22:03:04 2013 +0000<br>
<br>
Rename PageItem's Width/Height, some more rearranging of
pageitem.h<br>
<br>
git-svn-id: svn://scribus.net/trunk/Scribus@18095
11d20701-8431-0410-a711-e3c959e3b870<br>
<br>
diff --git a/scribus/pageitem_group.cpp b/scribus/pageitem_group.cpp<br>
--- a/scribus/pageitem_group.cpp<br>
+++ b/scribus/pageitem_group.cpp<br>
@@ -190,1 +190,1 @@<br>
- p->scale(Width / groupWidth, Height / groupHeight);<br>
+ p->scale(m_width / groupWidth, m_height / groupHeight);<br>
<br>
commit 697817116aa6a550bd648e49a24ec499184e5d67<br>
Author: Jean Ghali <a class="moz-txt-link-rfc2396E" href="mailto:jghali@libertysurf.fr"><jghali@libertysurf.fr></a><br>
Date: Fri Feb 10 22:20:30 2012 +0000<br>
<br>
some refactoring of graphics related function :<br>
- improve readability by removing some unnecessary indentation
levels<br>
- split ScPainter::drawVPath() functions<br>
- remove unused fillPathText() and strokePathText() function
from ScPainter<br>
<br>
git-svn-id: svn://scribus.net/trunk/Scribus@17288
11d20701-8431-0410-a711-e3c959e3b870<br>
<br>
diff --git a/scribus/pageitem_group.cpp b/scribus/pageitem_group.cpp<br>
--- a/scribus/pageitem_group.cpp<br>
+++ b/scribus/pageitem_group.cpp<br>
@@ -237,0 +190,1 @@<br>
+ p->scale(Width / groupWidth, Height / groupHeight);<br>
<br>
<br>
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:<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<span class="blob-code-inner">112 - p-><span class="pl-c1">scale</span>(Width
/ pat.<span class="pl-smi">width</span>, Height / pat.<span
class="pl-smi">height</span>);<br>
<br>
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!<br>
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!<br>
<br>
<br>
<br>
<br>
<br>
<br>
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?<br>
<br>
what i'd like to suggest you and the team:<br>
<br>
</span><span class="pl-en">PageItem_Group::DrawObj_Item</span>()
does urgently need some refactoring!<br>
<br>
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...<br>
<br>
so, how can we then document what DrawObj_item() is doing?<br>
for sure by writing some doxygen doc for the method.<br>
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.<br>
<br>
<br>
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<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
p-><span class="pl-c1">scale</span>(m_width / groupWidth,
m_height / groupHeight);<br>
<br>
so that it's clear what that line does?<br>
<br>
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.<br>
the refactoring should be submitted as a separate patch.<br>
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.<br>
<br>
of course, the patch for refactoring the existing method, should be
submitted even if the bug was not there :-)<br>
<br>
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.<br>
<br>
<br>
and my proposal to HOST:<br>
<br>
- 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 :-)<br>
<br>
- 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)<br>
<br>
<br>
<br>
voilą, that was a long email.<br>
i hope that it was readable even if i wrote it in one go.<br>
and i hope that this post can be useful to make the scribus code
base better and better in the future!<br>
<br>
<br>
have a nice day<br>
a.l.e<br>
</body>
</html>