Re: [Elementary-dev-community] Review + Asisstentece for Scratch required!
It should be fixed now, I have done with the cost of reopening a less serious bug https://bugs.launchpad.net/scratch/+bug/1046534 merge was proposed : https://code.launchpad.net/~darcy-develin/scratch/fix-1048186/+merge/123499 On Mon, 2012-09-10 at 00:08 +0400, Sergey "Shnatsel" Davidoff wrote: > Guys, it's great that you're going to invest in code quality. I didn't > tell you that, but it might be a good idea to throw in some unit > testing too, since Scratch is being used as a shared component and > there are lots of combos of all the autosave and autoupdate options so > it's hard to test them all manually. > > By the way, any idea what causes http://pad.lv/1048186 ? Scratch is > hardly usable for me because of it, I had to fall back to Gedit for > the first time in months. > -- Darcy Brás da Silva signature.asc Description: This is a digitally signed message part -- 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
Re: [Elementary-dev-community] Review + Asisstentece for Scratch required!
Guys, it's great that you're going to invest in code quality. I didn't tell you that, but it might be a good idea to throw in some unit testing too, since Scratch is being used as a shared component and there are lots of combos of all the autosave and autoupdate options so it's hard to test them all manually. By the way, any idea what causes http://pad.lv/1048186 ? Scratch is hardly usable for me because of it, I had to fall back to Gedit for the first time in months. -- Sergey "Shnatsel" Davidoff OS architect @ elementary -- 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
Re: [Elementary-dev-community] Review + Asisstentece for Scratch required!
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
> 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 pre
Re: [Elementary-dev-community] Review + Asisstentece for Scratch required!
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 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 requeste
Re: [Elementary-dev-community] Review + Asisstentece for Scratch required!
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
signature.asc
Description:
Re: [Elementary-dev-community] Review + Asisstentece for Scratch required!
I'd like some examples too, but this sounds like some strange behaviour indeed. This happens many times with huge projects. On Sun, Sep 9, 2012 at 1:02 PM, Sergey "Shnatsel" Davidoff < ser...@elementaryos.org> 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 > > -- > 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
Re: [Elementary-dev-community] Review + Asisstentece for Scratch required!
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 -- 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
