[scribus] "modernizing" / sanitizing the code conventions for scribus

[ale rimoldi]

buondì,

today i have been working on a (small) feature that i might be able to
present tomorrow and, while struggling with
- a function called ReadElem() that writes the element of the document
and
- the one called WriteElem() that returns a string with the element,
i recalled that some time ago i wanted to write a mail and ask for a
reform of the scribus code habits and code conventions.

here is a second try.

the goal of this proposition is to take into consideration the changes
that have happened in programming and programmers during the last 10
years... and try to make scribus programming just a bit more palatable
for new contributors.
(of course, there are more important things to discuss and work on...
but the topics presented below seem to be low hanging fruits...
changes that in the long term could lead to noticeable improvements in
the project)

here are my ideas:

- always put curly brackets around code blocks!
if (true)
qDebug() << "always";
is a bug waiting for being triggered.
old code should be changed when somebody works on it.
no need for mass changes.

- to avoid that the change above leads to even longer code, we should
use the mixed curly bracket style: classes and functions have
curly brackets on they own line
class Abc
{
void Abc
{
}
};
but loops and conditionals have "compact" curly brackets:
if (true) {
}
as far as i know, this convention is common in many communities.
old code does not have to be changed for now, but new code will
follow this new convention.
files can mix both styles. that's not a big issue.

- new files should indent by four spaces, not tabs.
old files can be slowly converted to spaces. on at a time.
there is no hurry.
no file should contain a mixed style, though.
(yes, that's a real burden: i've seen multiple patches with the wrong
spacing and, my self, have to invest five to ten minutes to clean up
my patches from all space indents...)

- if a function is longer than 50-60 lines, a patch/change cannot add
more than five lines to the function. (i have been working on a
function that is almost 200 lines long in a file that is 18'000 lines
long!)
(and i will have to refactor my patch to comply with this rule... but
i will be happy to do so : - )

- files should also get smaller... but that's a bit harder to enforce.

- file names should match the class name (it's not ok, that the
scribusXml.h file defines the ScriXmlDoc class)
from time to time a core developer could rename a file/class... we
could have a list of files that should be renamed...

- in the long term there should be only one prefix (if ever) for the
scribus files: currently we have "sc", "scr" (only very few) and
"scribus".
(but, really, no file inside of the scribus code base should have a
name that starts with the "scribus" (in any form) prefix.
we need a plan and it would be nice to see a file and/or a
class renamed from time to time... at least one per month?

- improve the naming of things. not so long ago somebody reported a
lower/upper case issue in a file name. the file being named with a
german word and the name not even matching the content of the
file. i would have expected the file name to be fixed for good, not
just the case being switched.
also, variables and function names should be as telling as possible
and not confuse the reader of the code:
- variables and function should never start with a capitalized letter
- generally speaking, they should be full words (or compositions of
words). ss, mdData, gridCols, fp4, ... don't seem to be good names
to me.
- they tell what is inside, not how they're built:

// QString ScriXmlDoc::WriteElem()
QList<PageItem*> emG;
QMap<int, PageItem*> emMap;
emG.clear();
for (int cor = 0; cor < selection->count(); ++cor)
{
emMap.insert(doc->Items->indexOf(selection->itemAt(cor)),
selection->itemAt(cor));
}
emG = emMap.values();

should probably become:

QList<PageItem*> getItemsFromSelection(selection) {
QList<PageItem*> result;
QMap<int, PageItem*> items;
for (auto item: selection.items()) {
items.insert(doc->Items->indexOf(item), item);
}
return items.values()
}

...

auto items = getItemsFromSelection(selection);


it's lot of work. many improvements have been made over the years,
but i really have the feeling that some "policy" changes and a
commitment for cleaner code are need.
even if introduced in an incremental way, this will help getting other
people on boat.
and it will make the life much easier to contributors like me, who
have to touch code written by other people.

i don't want to start a big discussion and waste everybody's time.
but i would be glad if some of the propositions could be accepted by
the team and even to shortly discuss some of the points.

i wish you all a wonderful night
a.l.e

___
Scribus Mailing List: ***@lists.scribus.net
Edit your options or unsubscribe:
http://lists.scribus.net/mailman/listinfo/scribus
See also:
http://wiki.scribus.net

[ZASKE Martin]

This is a well explained and graciously presented proposal.

I am not a programmer sadly but more than once, some help to my problems
has come from other users who have looked into the code.

I have learnt a little to read code for Android apps. And I could begin
to make sense from the "should probably become" part of the example
given. For looking myself, I would directly benefit from clearer file names.

And since ale is very very helpful on this list I want to say thank you
for the courage and the initiative to even suggest gentle reforms. It
does not sound like criticism on any other contributor nor on anything
that has been achieved in the past (and Scribus is still awesome, as is
my refrain in most of my mails).

I have not got a right, since I do not contribute, but I give my +1 for
ale as a fellow user and hero on this list.

fwiw and greetings,

Martin

Post by ale rimoldi
buondì,

___
Scribus Mailing List: ***@lists.scribus.net
Edit your options or unsubscribe:
http://lists.scribus.net/mailman/listinfo/scribus
See also:
http://wi