Editor Refactoring

[lukas-w]

Refactoring the editors' code. Introduced a new superclass for them providing a toolbar.
This is a try to make the code somewhat maintainable, but there's just so much done wrong there, it's still a long way.

[diizy]

I think our convention is to use CapitalCase for enum members...

[lukas-w]

I didn't invent that enum, I just moved it to public ;)

[diizy]

Yeah I know, just thought that since you're already refactoring things,
you wouldn't mind some extra work ;)

[lukas-w]

Sure, I'll do some style updates sooner or later ;) I think you renamed that particular enum in lmms2 already.

Btw, I suggest dropping the convention about parentheses being followed/preceded by a space character (like printf( "Hello World!" ); instead of printf("Hello World!");). It can get really hard to stick to, and it doesn't really add much to readability.

[diizy]

On 12/08/2014 09:55 PM, Lukas W wrote:

Sure, I'll do some style updates sooner or later ;) I think you
renamed that particular enum in |lmms2| already.

Did I? Can't remember if I've touched AutomationEditor in lmms2 yet...

Btw, I suggest dropping the convention about parentheses being
followed/preceded by a space character (like |printf( "Hello World!"
);| instead of |printf("Hello World!");|). It can get really hard to
stick to, and it doesn't really add much to readability.

Sorry but personally, I have to disagree with that. I actually think it
clears up the code a lot... it adds some separation between arguments
and it makes it a whole lot easier to read especially when there are a
lot of nested function calls. Or complex equations with lots of
parentheses. For example, compare this:

m_b1 = ( 4.0f * ( m_wc4 + sq_tmp1 - m_k4 - sq_tmp2 ) ) / m_a;
m_b2 = ( 6.0f * m_wc4 - 8.0f * wc2 * k2 + 6.0f * m_k4 ) / m_a;
m_b3 = ( 4.0f * ( m_wc4 - sq_tmp1 + sq_tmp2 - m_k4 ) ) / m_a;
m_b4 = ( m_k4 - 2.0f * sq_tmp1 + m_wc4 - 2.0f * sq_tmp2 + 4.0f * wc2 * k2 ) / m_a;

to this:

b1=(4*(wc4+sq_tmp1-k4-sq_tmp2))/a_tmp;
b2=(6*wc4-8*wc2*k2+6*k4)/a_tmp;
b3=(4*(wc4-sq_tmp1+sq_tmp2-k4))/a_tmp;
b4=(k4-2*sq_tmp1+wc4-2*sq_tmp2+4*wc2*k2)/a_tmp;

The first one looks a whole lot more readable to me...

[lukas-w]

I never suggested removing spaces between operators.

[tresf]

dropping the convention about parentheses being followed/preceded by a space character

My vote shouldn't really count here (since I contribute very little new C/C++ code) but as you can see from my last pull request, this bites me a lot too. 👍

-Tres

[lukas-w]

I think your vote should count. I updated the Coding Conventions in the wiki. :)

[diizy]

I think this issue needs some more discussion first (maybe on its own issue or the mailing list)... I think it's important we adhere to a style that is tolerable to all of us, especially to those of us that have to grind through a lot of code... but at the same time, we should maybe first try to get the whole codebase into a consistent style before we start changing it up again.

Perhaps as a compromise we could leave the spaces out when there's only one argument in a function call, or no nested calls, or for things like casts that only have a single item, but I really think the spaces should be kept in more complex calls/expressions.

[lukas-w]

we should maybe first try to get the whole codebase into a consistent style before we start changing it up again.

I disagree. At the current sate, the code already is ugly and inconsistent, and we're in the process of fixing this, which is not even half finished. Better agree on a final style while we're at it, not when we're finished and have to go through all the code again.

Let's not make this a big deal. I only removed the requirement of adding spaces inside parenthesis. For instance, this:

void setParent( QObject* parent );

is not really easier to read or understand than

void setParent(QObject* parent);

but it takes twice the time to write and many editors don't like it.

I'm not saying we should never use spaces. Of course there are complex cases where adding spaces around the arguments can improve readability.

[diizy]

On 12/09/2014 12:22 AM, Lukas W wrote:

I'm not saying we should never use spaces. Of course there are complex
cases where adding spaces around the arguments can improve readability.

So maybe we should do like I suggested, and write it down that it's
allowed to leave the spaces out for simple function calls etc., but
include them when there are nested calls, complex equations or
calls/declarations with lots of arguments. Should be good for everyone?
I'm not trying to be difficult here, but I do have to read a lot of the
code people submit...

And since we're apparently talking about coding style guidelines, a
couple of other things I'd like written down:

• Always put opening braces on a newline, never at the end of the
  previous line (I think this may already be in the guidelines, but had to
  mention it because it really throws me off when I read some code, I need
  the structure of symmetrically placed braces)
• The one exception: when the content inside the braces is short enough
  to be one-lined, then the braces can all be on the same line
• Ternary operators: I think complex ternary expressions should be
  formatted as follows:

x = condition
? value
: othervalue;

• The exception: for simple expressions, it's ok to inline all of it on
  one line, provided it doesn't make the line too long:

x = condition ? value : othervalue;

• Again this might be in the guidelines already, can't remember, but if
  it isn't it should be: always spaces between all operators. For this,
  there can be no exceptions. I simply loathe any coding style that leaves
  out spaces between operators/values in equations... it literally makes
  my brain hurt.

Does this all sound reasonable?

[lukas-w]

write it down that it's allowed to leave the spaces out for simple function calls etc., but include them when there are nested calls, complex equations or calls/declarations with lots of arguments. Should be good for everyone?

Sure, even though I consider this to be something that goes without saying.

Always put opening braces on a newline, never at the end of the previous line

I'm not sure what you mean. Is it this:

void main(int argc, const char*[] argv)
{
    ...
}

vs this?

void main(int argc, const char*[] argv) {
    ...
}

If that's what you mean, this is in the coding conventions, but only for if else for while statements.

Ternary operators: I think complex ternary expressions should be formatted as follows:

Not at all, I think complex ternary expressions should generally be avoided. Use if/else instead.

always spaces between all operators

This is in the guidelines already ;)

[diizy]

On 12/09/2014 01:10 AM, Lukas W wrote:

write it down that it's allowed to leave the spaces out for simple
function calls etc., but include them when there are nested calls,
complex equations or calls/declarations with lots of arguments.
Should be good for everyone?

Sure, even though I consider this to be something that goes without
saying.

Cool. While it might seem obvious to you or me, there's lots of people
(particularly people new to coding) for whom these things are not so
obvious, so it's always better to write them down explicitly...

Always put opening braces on a newline, never at the end of the
previous line

I'm not sure what you mean. Is it this:

void main(int argc, const char*[] argv)
{
...
}

vs this?

void main(int argc, const char*[] argv) {
...
}

If that's what you mean, this is in the coding conventions, but only
for |if else for while| statements.

Yes, that. I think we should just edit it to apply to all curly braces.
For consistency.

Ternary operators: I think complex ternary expressions should be
formatted as follows:

Not at all, complex ternary expressions should generally be avoided.
Use if/else instead.

Hm, I'm not so sure they should be. Ternary expressions can sometimes be
more suitable, eg. in cases where you set the same variable in both
cases. Makes for more streamlined code. And if properly formatted,
they're just as readable as if/else.

always spaces between all operators

This is in the guidelines already ;)

Cool.

[diizy]

Is this still WIP?

Seems like some kind of conflict here, you may need to rebase this...

[lukas-w]

Yes it's still in progress. I expected those merge conflicts, I'll fix them soon (already did twice in this PR).

[lukas-w]

The last few commits (0df3998, 1ee9340, e0dbfa6) might be interesting for developers. I moved most GUI related code in the Engine class to a new singleton class called GuiApplication.
Code like

Engine::mainWindow()->workspace();

is now

gui->mainWindow()->workspace();

Of course this should check whether gui != nullptr first.

[lukas-w]

Merging as it becomes too difficult to resolve the conflicts.