elementary-dev-community team mailing list archive

[Darcy Brás da Silva <dardevelin@xxxxxxxxxxxxxx>]

I indeed can, sorry for taking long, I had to sleep a bit, couldn't
think straight after a code marathon :).

Instead of pasting code in here I will link to the respective lines and
explain a bit what's wrong with what.

For instance: 

from-line:77 to: 88 {Services/Document.vala}
http://bazaar.launchpad.net/~elementary-apps/scratch/scratch/view/head:/src/Services/Document.vala#L77

line 86 
return _state; 

various problems:

1 - Never Runs due to else statement on line 84 which covers the last
	possible scenario.
2 - This creates the bug https://bugs.launchpad.net/scratch/+bug/1038291

Where the correction is not very clear. For some reason returning _state
fixes it but then we get into another problem, which is

We return _state without a way of knowing that _state was assigned before

Just in this file we have more hairy stuff, lets see,

line 67 : public string filename {get; public set; }
so anyone can change this variable ?
most functions using this variable use it directly and they check it, 
if it's not null, they might perform checks in an unassigned situation
which can become a bug witch hunt.

some proof of what I just mentioned
line 111: http://bazaar.launchpad.net/~elementary-apps/scratch/scratch/view/head:/src/Services/Document.vala#L111

relies on the previously shown `buggy' line : http://bazaar.launchpad.net/~elementary-apps/scratch/scratch/view/head:/src/Services/Document.vala#L77
http://bazaar.launchpad.net/~elementary-apps/scratch/scratch/view/head:/src/Services/Document.vala#L273

And is also incoherent, if we just want state, shouldn't we just use the private member, which should be storing the current state of it ?
why do we keep requesting a full check ?

line http://bazaar.launchpad.net/~elementary-apps/scratch/scratch/view/head:/src/Services/Document.vala#L719

this one is so precious that I'm posting it all:
the function had no comments, so the ones there are my notes

 public bool can_write () {

	     //check that we never know if the var is set            
            if (filename != null) {

                FileInfo info;
                bool writable;

                try {

                    info = file.query_info (FileAttribute.ACCESS_CAN_WRITE, FileQueryInfoFlags.NONE, null);
                    writable = info.get_attribute_boolean (FileAttribute.ACCESS_CAN_WRITE);

                    return writable;

                } catch (Error e) {

                    warning ("%s", e.message);
		    //redundancy, this is writable variable job
                    return false;

                }

            } else {
		//heres were we win, so if filename == null, we can_write to the file
                return true;

            }

        }

line 697: http://bazaar.launchpad.net/~elementary-apps/scratch/scratch/view/head:/src/Services/Document.vala#L696

Both error on line 708 and filename set to null the return is 0
Note: 0 is a valid size, while one want's indicate an error scenario
so -1 and -2 would be more appropriate. 
Probably trowing an error would also be a good idea.

file Scratch.Vala
this one was introduced by me actually, but is something that can be easily fixed.

http://bazaar.launchpad.net/~elementary-apps/scratch/scratch/view/head:/src/Scratch.vala#L135

I did not handled File exception which now make scratch crash if the decoding of filename files.
but to be fair, this tactic is wrong since the very start. 
The open_file function should get a file and this would not be required.
However i don't know every part of the code that uses this, and some of what 
I saw do have a legitime reason to send an URI instead of a File (like drag & drop ).
Since the moment of the patch I explicitly told that it was also inefficient 
so yeah...

However that does require to be handled with a try-catch.... 
We run a bit out of options if we fail the access to a name though.

line http://bazaar.launchpad.net/~elementary-apps/scratch/scratch/view/head:/src/Widgets/Tab.vala#L145

get_basename to a presentation element
remind you valadoc's where it says that it does NOT return a UTF-8 string
so improper for that.

I also noted that most file check happen at filename level, which is wrong, one can possibly have
two files named the same from different paths, and so unique id of the files should be made based 
on full path and not just filename
Maybe an hash don't know... needs checking.

I think this is enough for the sample requested. Isn't it ? 
I can go find more if required though.

PS: I have also found Many CGL violations... 

Hope to hear from all soon.
cheers 

On Sun, 2012-09-09 at 16:02 +0400, Sergey "Shnatsel" Davidoff wrote:
> Could you provide a few examples of the suspicious code/behaviour?
> 
> I'm no dev, but as a script kitty I can tell that grep is your best
> friend when it comes to locating function calls in the code.
> -- 
> Sergey "Shnatsel" Davidoff

-- 
Darcy Brás da Silva <dardevelin@xxxxxxxxxxxxxx>

Attachment: signature.asc
Description: This is a digitally signed message part