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