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 <[email protected]> > 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 <[email protected]> > > > > -- > > Mailing list: https://launchpad.net/~elementary-dev-community > > Post to : [email protected] > > Unsubscribe : https://launchpad.net/~elementary-dev-community > > More help : https://help.launchpad.net/ListHelp
-- Darcy Brás da Silva <[email protected]>
signature.asc
Description: This is a digitally signed message part
-- Mailing list: https://launchpad.net/~elementary-dev-community Post to : [email protected] Unsubscribe : https://launchpad.net/~elementary-dev-community More help : https://help.launchpad.net/ListHelp
