elementary-dev-community team mailing list archive

[Mario Guerriero <mefrio.g@xxxxxxxxx>]

Scratch is becoming every push a bigger project which make it a bit more difficult to maintain.

I'll return home Wednesday, please wait me to discuss about it :)

Thanks for your love on the project. 

Mario Guerriero
Sent from iPhone 3GS

On 09/set/2012, at 18:08, Darcy Brás da Silva <dardevelin@xxxxxxxxxxxxxx> wrote:

> 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>
> 
> -- 
> Mailing list: https://launchpad.net/~elementary-dev-community
> Post to     : elementary-dev-community@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~elementary-dev-community
> More help   : https://help.launchpad.net/ListHelp