[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