<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>