[gtk3] reformat code request

[hanspr]

Hi @gfrenoy

I keep finding more nasty bugs in Ásbru, and I would like to be able to contribute more code, but the following issues make that effort complicated for me. And maybe others.

I have coded with Perl for more than 20 years now, and still:

• I find this code is extremely obscure
• Formatting is a big issue, lots of multiple statements stacked in a single line.
  • This makes tracing and commenting code to test a real problem more complicated.
• Of course, very poor documentation on what part of anything does.
  • I have found paces of code and I do not there to document my findings (so I do not introduce more complications on an already complex code). And this means that any new comer will have to go through the same struggle just to figure out where is an event happening.

You ether have been too respectful of the coding style of the original author, or you are a Perl monk from the highest order.

I do not want to be misunderstood, you are doing a great job, but for simple mind coder like me is very difficult to contribute and help you some times.

Now the proactive part,

I'm willing to reformat all the code without moving any logic to it and send a pull request for revising. To see if we can find a code style that could help understand, trace and debug the code easier.

And begin documenting on the wiki section the code technical details of it (or try to do so), may be on the process I can fix more bugs.

This could help us add comment references to the sections in the wiki where we can find detailed information of the code, without bloating the source code with comments.

Its Ok, If you think that this is not necessary. I will adapt, and keep trying to contribute as much as I can, but I feel the learning curve is too steep.

[gfrenoy]

Hi @hanspr,

The code we are maintaining has been created by David Torrejon Vaquerizas. He did a wonderful job and we are all there because of the energy he put into this project.

On my side, I learned some Perl at school (it was last century ;-p) and since then, I did not write any line of Perl code myself so I'm not really comfortable with that language. So my "strategy" was indeed to keep David's style and not breaking something that (mostly) works.

I don't pretend it was a good or a bad strategy, it was simply the most obvious one to me to make sure we can keep things working without spending too much time on my side.

If we now have an experienced Perl developer on board, I would indeed strongly support your offer to reformat the code and make sure we have a strong code base to move forward and, hopefully, have more Perl developers contributing to this project.

The only "constraints" I would have are:

• have a clear coding standard to comply with
• make sure we separate the "reformatting" activity from any other activity

How can start from there ? Would you have a reference for a known Perl coding standard ? Can you do a single PR with all the "cosmetic" changes ?

In any case, thanks a lot for your help and collaboration, it is very much appreciated !

[hanspr]

I understand, David did an incredible tool, I sent a donation to his project many years back.

I don't claim to be an expert on Perl, have worked must of the time on web project, no GTK experience or very little.

There are not clear standards for Perl the motto, "there is more than one way to do it", says it all.

There is a formatting tool Perl::Tidy to try to impose some order on this.

I will create a new branch in my fork, and send you a proposal and see if we could get something from there.

[gfrenoy]

I will create a new branch in my fork, and send you a proposal and see if we could get something from there.

Looks great ! Thanks a lot :)

[egmontkob]

While at it:

The source code intensively uses TAB characters, assuming a width of 4. It uses TABs not only for indentation, but for alignment too.

See the most extreme example where there are 26(!) TAB characters after the else, before the command belonging to this branch.

People might configure their text edtiors so that TABs are 4 chars, but other contexts, such as GitHub's online diff viewer as linked above, or a local git diff, or just cat'ing, grep'ing etc. in the terminal will use 8, pushing the statement way too far to the right, potentially out of the regular viewport.

A couple of things you could do:

· Wrap the conditional commands into new lines, i.e. change

if (condition) foo;
else<TAB><TAB> bar;

into

if (contidion)
    foo;
else
    bar;

· Use TABs for indentation only and not for alignment. That is, TABs are only allowed at the beginning of lines (and still then, prefer spaces when aligning a function invocation's second and subsequent parameters under the first).

· Switch to indent by 8.

· Eliminate all TABs, use spaces only.

· Whatever else you prefer :)

This is just a weak recommendation from me, if you like the current indentation as-is now then I really don't mind if you keep it like this.

[KlaasT]

As there doesn't seem to be coding guidelines for Perl projects maybe the PHP PSR could ne some orientation

[hanspr]

Thanks to both for your comments.

My main concerns are the "if", "while", "foreach" one line blocks of code, excessive use of unless and reversed ifs statements. I know is part of the language, but we can all read a direct "if" and understand its intention at first glance, and some unless statements are double negations, like this

next unless $cmd ne '';

to

if ($cmd eq '') {
    next;
}

Or this one, if all this is ok, attacht a routine, unless better not.

( ( $$self{_UUID} ne '__PAC__QUICK__CONNECT__' ) && ( $$self{_UUID} ne '__PAC_SHELL__' ) && $$self{'_CFG'}{'defaults'}{'show screenshots'} ) and $$self{_TAKE_SCREENSHOT} = Glib::Timeout -> add_seconds( $$self{_CFG}{environments}{ $$self{_UUID} }{method} =~ /rdesktop|RDP/go ? 10 : 2, sub {
return 1 if ( ! $$self{CONNECTED} ) || ( ! $$self{_FOCUSED} );
.. MORE CODE ...
return 0;
} ) unless defined $$self{_TAKE_SCREENSHOT} || scalar( @{ $$self{_CFG}{environments}{ $$self{_UUID} }{screenshots} } );

2 or more statements in a single line very long line

$$self{_GUI}{_VTE} -> signal_connect( 'commit' => sub { $$self{_CFG}{'defaults'}{'record command history'} and $self -> _saveHistory( $_[1] ); $self -> _clusterCommit( @_ ); } );

to

$$self{_GUI}{_VTE} -> signal_connect( 'commit' => sub { $$self{_CFG}{'defaults'}{'record command history'} and $self -> _saveHistory( $_[1] );
$self -> _clusterCommit( @_ ); } );

simple reformat of one liner blocks

foreach my $line_num ( sort { $a <=> $b } keys %found ) { push( @{ $w{window}{gui}{treefound}{data} }, [ $line_num, $found{$line_num} ] ); }

to this

foreach my $line_num ( sort { $a <=> $b } keys %found ) {
    push( @{ $w{window}{gui}{treefound}{data} }, [ $line_num, $found{$line_num} ] );
}

from this

while ( -f $$self{_TMPPIPE} ) { $$self{_TMPPIPE} = $CFG_DIR . '/tmp/pac_PID' . $$ . '_n' . ++$_C . '.pipe'; }

to a perl like string, that is shorter and cleaner to read

while ( -f $$self{_TMPPIPE} ) {
    ++$_C;
    $$self{_TMPPIPE} = "$CFG_DIR/tmp/pac_PID{$$}_n$C_.pipe";
}

These are basically the suggestions I have, and if I find a reliable way to shorten some lines even better.

@egmontkob, tabs to spaces (point taken) I guess the use of tabs is deeply hard wired in some of us that began coding on a Apple 2 with 48Kb of ram and a 128Kb floppy, we could no waste any bytes on spaces to indent :-)

[gfrenoy]

Thanks all for your comments and contributions :) I'm really with you on this, the current coding standard (tabs, indentation, ...) is old fashioned ! Let's pick a modern one and let's move to it. It will take time and efforts but if we keep our motivation, I'm convinced it will help :-)

So I get back to my first point : what will be our standard ?

Can't we reuse something that exists ?

I searched a bit and ended up here that basically suggests to use perlstyle and Perl::Tidy.

Is this a good starting point ?

[hanspr]

Hi @gfrenoy

I'm happy to see the enthusiasm on this, you have pushed me to release this first preview earlier that expected.

First I will state the objective: Facilitate any coder the possibility to read the code an tweak Ásbru to there needs without any knowledge of Perl.

Perl Tidy is nice, if we agree on the rules to apply, I can tell from the code that David used perl tidy, so it is not the final solution but it will help.

Lets begin with something simple:

• Use spaces as indentation (suggested by @egmontkob) 4 Spaces per indentation
• No trailing white spaces if possible
• No use of:
  • unless
  • reverse ifs
  • non explicit ifs
  • use strings like perl intends to be used. With interpolation
• Parenthesis can be together unless you feel that a separation makes code more readable.
  • Excess spaces also create long lines an not really a readable code
• All conditional blocks, Loops must be on different lines, even if it is one statement in the block
• Local variables should be declared at the beginning of a function block, and if they are not intialized, they could be declared as one line.
  • my ($var1, $var2, $var3);
  • Avoid declaring them in the middle of the block, unless is going to be used as a temporary variable inside an inner block
• Rules might be bent from time to time, when really brings something valuable to the code

These are very classic rules, but work.

Perl Tidy, will help with the formating: spaces, no trainling spaces, indentations and so on. But any normal editor will do so too.

But Perl Tidy it will not convert implicit ifs into ifs, remove unless statements, etc.

Now brief example of what I'm proposing and the benefit of passing over the code one more time.

I have released the branch gtk3format on my fork: https://github.com/hanspr/asbru-cm/tree/gtk3format

I was planning on reformat a little bit more code, but this gives also the opportunity to show what I propose and make changes as you all may suggest.

I began from the beginning with asbru-cm

You may do a compare between branches to see the difference.

• I removed all the one liners and substituted for real ifs.
• I saw the opportunity to group many one liners inside loops
• And related to issue Migrate config files #182 I removed the hard coded $CFG_DIR variables of all modules to read the global variable from asbru-com
  • This will give as the opportunity to have 2 versions of Ásbru installed (a working one) and a test one, without conflicts in the configuration files.
  • The first time I tried to change the configuration file to do so, I ended up mixing and destroying my configurations because I was not aware of the hard coded configurations in all the modules.
  • It also brings the opportunity to migrate from one configuration to another on different version releases. For example when the gtk3 branch is ready, we could move config/pac to config/asbru

Finally as I moved over the code I was documenting what I saw : https://github.com/hanspr/asbru-cm/wiki

It is very poor right now, but I hope I can make it grow a little bit each week.

Is possible that all of you, that have more time on this project, are more familiar with this information but to me is very new and I think that collecting all the knowledge we gather in a single place will help us out when we need to solve something.

Reformatting the code from here on will get more time consuming and tricky. And perhaps we could do a little part as we fix a bug here and there like we are doing now.

Well hope I explained my self correctly.

And if you think is not worth the effort say so, I will not get offended or turn around.

Wait for your comments. Tell me if you believe that it is or not more readable, and clear to follow, even with the new loops.

[gfrenoy]

Looks great :) No need to be perfect at this stage ; we'll continue this effort as we are bringing new features...

From here:

Can we have CODING_STANDARD.md file at the root level with the rules you are proposing ?

Can we please separate the functional changes from that activity ? I mean: we need one single huge commit with all the "cosmetic" changes and then we'll have another one for #182 (or any other great contribution you would do ;p).

About documentation, I would prefer having this into the code itself and autogenerated (I know doxygen but there's likely something specific for Perl...). This is usually easier to maintain than yet another document to update after you changed the code.

Thanks again for your great help. Much appreciated !

[egmontkob]

and some unless statements are double negations, like this

next unless $cmd ne '';

LOL. Perl folks seem to love this style. I agree with you, I find it way too twisted. Unfortunately GLib/GTK guys seem to love it too, even in VTE's source there's plenty of

g_return_if_fail (foo != bar);

which make my brain explode.

Speaking of negations, another thing I firmly dislike is UI checkboxes with the label "Disable blahblah". Quickly going through Ásbrú's config option I haven't found any, but if you come across one, please invert it to say "Enable" :-)

[hanspr]

@gfrenoy

1. OK, during the day
2. OK, will take note of enhancements and do them in a second pass.

Would you prefer that I reformat a file and then add a pool request of that single file?

So it is easier to see if by mistake I break something during the process, it will be easier to blame that particular file and fix the mistake?

3. Will added in documentation, basically

The intention part can be documented close to the lines of code with the normal use of the hashtag #

The perl compiler stops when it finds this marker __END__

Should be placed alone in a single line at the beginning (of the line, no indentations)

After this tag, you can freely write whatever you want, with no restrictions.

Perl uses the pod format, to write this documentation. Plain old documentation format. It is another markup standard

So large documentation, is usually found at the bottom in perl scripts.

When used properly you can use the command perldoc to extract and format the documentation from a script and printed on screen or exported to other formats. html, markup, etc.

I will include a summary of the pod documentation format and the basic style that is how all cpan is documented.

[KlaasT]

Pod or Doxygen syntax would be great. With this I could create some travis jobs to generate documentation files in HTML from it.

[gfrenoy]

Would you prefer that I reformat a file and then add a pool request of that single file?

One pull request for all files that have be reformatted is better. But without any logical change.

, it will be easier to blame that particular file and fix the mistake?

Exactly ;-)

Thanks !

[KlaasT]

About documentation, I would prefer having this into the code itself and autogenerated (I know doxygen but there's likely something specific for Perl...). This is usually easier to maintain than yet another document to update after you changed the code.

@gfrenoy As stated earlier I'm already working on autogenerating docs from the work hanspr is doing. Please just give me some time to finish it ;-)

[hanspr]

@gfrenoy

Want to inform that I realised today that from this moment on I will have to do 2 pull requests per file.

The first one will have only the cosmetic changes: removing tabs, extra spaces, etc. This first commit will have all the large differences that make looking for changes hard

In the second pull request, I will send the changes to the implementations on ifs, unless, one liners, etc. So those changes are clear and can be compared in case of a mistake.

[hanspr]

@gfrenoy

While reimplementing the code to make it readable, I'm finding ifs that have a logic that should be revised because the logic repeats several times or some other details.

As we agreed I will not change the logic right now, but how do you want me to mark this code so anyone can revise it latter, what kind of comment is best for this situations.

# TODO : Comment

# FIX ME : Comment

# FOR REVISION : Comment

[gfrenoy]

Hi Hans,

Yes, good idea :) Thanks. As we have already some of them, can you use FIXME (without space) ?

And FOR REVISION is maybe not necessary, you can just use TODO... I mean: what is "to do" is to review what is commented...

[hanspr]

@gfrenoy

Regarding this issue, I have applied full coding standard, and added some documentation to:

*asbru-cm
*lib/PACMain.pm
*lib/PACTerminal.pm
*lib/pac_conn
*lib/PACUtils.pm

I thinks this are the must important files we are working on right now.

The rest of the files I applied only a large cosmetic change to remove tabs and extra spaces. I will try to deliver a full standard for those every week or two, or early if I have to fix a bug in one off them.

Documentation.

Is not complete, I added what I could understand with a quick overview, and the information that I thought it could be relevant to know for a new programmer that wants to get involved.

You been digging into this code longer, if you have information that should be added, please complete it so we can grow the information on how all this works. I still haven't spend enough time to follow the internal communication with sockets that Ásbrú makes within PACTerminal and PACMain using the sockets.

I have a issues and fixes that I noticed during the process of reading and reformatting the code, and I will try to add those fixes to new pull requests in the next days.

Passing through the code I noticed in PACUtils.pm that there is a small DONATORS_LIST, never expected that my donation would give me the honor to be there under my company name (Microflow Software SA de CV), did not want to fix the misspell though.

I don't know if you want to close this issue to clean up the board.

[gfrenoy]

Again a huge thanks ! It makes thing way more pleasant to work on now ;-)

And ok, let's close this for now and we know that minor improvements will continue to be made to move to the standard as much as possible.

About the donor's list ... Not sure that this is the best place to have it but it does not hurt :p