elementary-dev-community team mailing list archive

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

No problem, I'm looking forward to work with you :)
I can wait a couple days. While doing so I'll try prepare some possible
solutions :)
On Sun, 2012-09-09 at 18:23 +0100, Mario Guerriero wrote:
> 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

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

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