***important*** cannot remove a class anymore...
Hi I do not know if this is linked to recent changes but we cannot remove classes or move them to another package. Stef
Hi, This is a side effect of https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement... (which was reviewed by two people, so not obvious). What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop. Should be easy to fix for the people involved in case 16475. Marcus On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr> wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org http://www.marcusdenker.de
On 05 Sep 2015, at 09:29, Marcus Denker <marcus.denker@inria.fr> wrote:
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement... <https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...>
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
I added an issue tracker entry: https://pharo.fogbugz.com/f/cases/16484/Endless-loop-on-refresh-after-deleti... <https://pharo.fogbugz.com/f/cases/16484/Endless-loop-on-refresh-after-deleti...> Marcus
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice. Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system. If we require more, we will be back at a process where due to Fear we do nothing.
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement... <https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...>
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
I will take a look now!
On 05 Sep 2015, at 11:48, Marcus Denker <marcus.denker@inria.fr> wrote:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement... <https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...>
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
As a side note: can we somehow write UI tests? Because this use-case could be implemented as a test to ensure future auto checks. Uko
On 05 Sep 2015, at 12:42, Yuriy Tymchuk <yuriy.tymchuk@me.com> wrote:
I will take a look now!
On 05 Sep 2015, at 11:48, Marcus Denker <marcus.denker@inria.fr <mailto:marcus.denker@inria.fr>> wrote:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement... <https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...>
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
Iâm making progress. So the issue is that the selectedClass variable of Nautilus (model) is not set to nil when the class is being removed. I will check why.
On 05 Sep 2015, at 12:44, Yuriy Tymchuk <yuriy.tymchuk@me.com> wrote:
As a side note: can we somehow write UI tests? Because this use-case could be implemented as a test to ensure future auto checks.
Uko
On 05 Sep 2015, at 12:42, Yuriy Tymchuk <yuriy.tymchuk@me.com <mailto:yuriy.tymchuk@me.com>> wrote:
I will take a look now!
On 05 Sep 2015, at 11:48, Marcus Denker <marcus.denker@inria.fr <mailto:marcus.denker@inria.fr>> wrote:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement... <https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...>
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
Is there an easy way to browse a history of something. Or a blame view? Because âbrowse versionsâ that is available in Nautilus shows only the modifications in the current image. Having quick access to the history can speedup things sooo much
On 05 Sep 2015, at 12:59, Yuriy Tymchuk <yuriy.tymchuk@me.com> wrote:
Iâm making progress. So the issue is that the selectedClass variable of Nautilus (model) is not set to nil when the class is being removed. I will check why.
On 05 Sep 2015, at 12:44, Yuriy Tymchuk <yuriy.tymchuk@me.com <mailto:yuriy.tymchuk@me.com>> wrote:
As a side note: can we somehow write UI tests? Because this use-case could be implemented as a test to ensure future auto checks.
Uko
On 05 Sep 2015, at 12:42, Yuriy Tymchuk <yuriy.tymchuk@me.com <mailto:yuriy.tymchuk@me.com>> wrote:
I will take a look now!
On 05 Sep 2015, at 11:48, Marcus Denker <marcus.denker@inria.fr <mailto:marcus.denker@inria.fr>> wrote:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement... <https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...>
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
On 05 Sep 2015, at 13:07, Yuriy Tymchuk <yuriy.tymchuk@me.com> wrote:
Is there an easy way to browse a history of something. Or a blame view? Because âbrowse versionsâ that is available in Nautilus shows only the modifications in the current image. Having quick access to the history can speedup things sooo much
It should still show all changed done since the last condense of the sources/rewrite of the .sources. But yes, the âversionsâ that is not using the data from the revision control system is kind of useless⦠Marcus
This is my all point. I cannot help because I cannot see simply what you did. Le 5/9/15 13:07, Yuriy Tymchuk a écrit :
Is there an easy way to browse a history of something. Or a blame view? Because âbrowse versionsâ that is available in Nautilus shows only the modifications in the current image. Having quick access to the history can speedup things sooo much
On 05 Sep 2015, at 12:59, Yuriy Tymchuk <yuriy.tymchuk@me.com <mailto:yuriy.tymchuk@me.com>> wrote:
Iâm making progress. So the issue is that the selectedClass variable of Nautilus (model) is not set to nil when the class is being removed. I will check why.
On 05 Sep 2015, at 12:44, Yuriy Tymchuk <yuriy.tymchuk@me.com <mailto:yuriy.tymchuk@me.com>> wrote:
As a side note: can we somehow write UI tests? Because this use-case could be implemented as a test to ensure future auto checks.
Uko
On 05 Sep 2015, at 12:42, Yuriy Tymchuk <yuriy.tymchuk@me.com <mailto:yuriy.tymchuk@me.com>> wrote:
I will take a look now!
On 05 Sep 2015, at 11:48, Marcus Denker <marcus.denker@inria.fr <mailto:marcus.denker@inria.fr>> wrote:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
Ok, so the deepest Iâve got is that âself classWidget getClassesListâ (where self is NautilusUI) always returns an empty list, so removed class is not found in that list, so it is not removed from the model. I have not changes ClassWidget and never worked with it, so from now the investigation will go slower. Iâd appreciate help in any Nautilus people are out there. Uko
On 05 Sep 2015, at 12:59, Yuriy Tymchuk <yuriy.tymchuk@me.com> wrote:
Iâm making progress. So the issue is that the selectedClass variable of Nautilus (model) is not set to nil when the class is being removed. I will check why.
On 05 Sep 2015, at 12:44, Yuriy Tymchuk <yuriy.tymchuk@me.com <mailto:yuriy.tymchuk@me.com>> wrote:
As a side note: can we somehow write UI tests? Because this use-case could be implemented as a test to ensure future auto checks.
Uko
On 05 Sep 2015, at 12:42, Yuriy Tymchuk <yuriy.tymchuk@me.com <mailto:yuriy.tymchuk@me.com>> wrote:
I will take a look now!
On 05 Sep 2015, at 11:48, Marcus Denker <marcus.denker@inria.fr <mailto:marcus.denker@inria.fr>> wrote:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement... <https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...>
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
Ok, so the deepest Iâve got is that âself classWidget getClassesListâ (where self is NautilusUI) always returns an empty list, so removed class is not found in that list, so it is not removed from the model. I have not changes ClassWidget and never worked with it, so from now the investigation will go slower. Iâd appreciate help in any Nautilus people are out there.
this morning I started to read the method of AbstractNautilusUI now I still do not get what you did. I hate that I have to download a special image and merge a special slice. We should really have a diff in plain text as it is done in squeak sent to this mailing-list. With that it would take me less than a second to see what you were doing. Stef
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
I do not want to require more. I want to see all the changes that we are doing without having to download an image and doing merge. I read many squeak changes like that and IT WORKS. Stef
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
You can see changes for that patch here: https://github.com/pharo-project/pharo-core/commit/2536752ecdd88e15b00c6d3e2...
On 05 Sep 2015, at 13:23, stepharo <stepharo@free.fr> wrote:
On 05 Sep 2015, at 11:40, stepharo < <mailto:stepharo@free.fr>stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
I do not want to require more. I want to see all the changes that we are doing without having to download an image and doing merge. I read many squeak changes like that and IT WORKS.
Stef
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement... <https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...>
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo < <mailto:stepharo@free.fr>stepharo@free.fr <mailto:stepharo@free.fr>> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
On 05 Sep 2015, at 13:29, Yuriy Tymchuk <yuriy.tymchuk@me.com> wrote:
You can see changes for that patch here: https://github.com/pharo-project/pharo-core/commit/2536752ecdd88e15b00c6d3e2...
YES, we have that for years now. True, it is one click away, but the view is quite nice (not something that can be reproduced in an email).
On 05 Sep 2015, at 13:23, stepharo <stepharo@free.fr> wrote:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
I do not want to require more. I want to see all the changes that we are doing without having to download an image and doing merge. I read many squeak changes like that and IT WORKS.
Stef
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org http://www.marcusdenker.de
Fix is under monkey validation: https://pharo.fogbugz.com/f/cases/16485/Class-panes-not-refreshed-when-chang... <https://pharo.fogbugz.com/f/cases/16485/Class-panes-not-refreshed-when-chang...>
On 05 Sep 2015, at 14:26, Sven Van Caekenberghe <sven@stfx.eu> wrote:
On 05 Sep 2015, at 13:29, Yuriy Tymchuk <yuriy.tymchuk@me.com> wrote:
You can see changes for that patch here: https://github.com/pharo-project/pharo-core/commit/2536752ecdd88e15b00c6d3e2...
YES, we have that for years now.
True, it is one click away, but the view is quite nice (not something that can be reproduced in an email).
On 05 Sep 2015, at 13:23, stepharo <stepharo@free.fr> wrote:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
I do not want to require more. I want to see all the changes that we are doing without having to download an image and doing merge. I read many squeak changes like that and IT WORKS.
Stef
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org http://www.marcusdenker.de
On 05 Sep 2015, at 13:29, Yuriy Tymchuk <yuriy.tymchuk@me.com> wrote:
You can see changes for that patch here: https://github.com/pharo-project/pharo-core/commit/2536752ecdd88e15b00c6d3e2... YES, we have that for years now.
and how many people read one change?
True, it is one click away, but the view is quite nice (not something that can be reproduced in an email). No it is not because I delete the mail with these headers and I cannot find a list on the web to find the diff.
On 05 Sep 2015, at 13:23, stepharo <stepharo@free.fr> wrote:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing. I do not want to require more. I want to see all the changes that we are doing without having to download an image and doing merge. I read many squeak changes like that and IT WORKS.
Stef
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org http://www.marcusdenker.de
2015-09-05 11:48 GMT+02:00 Marcus Denker <marcus.denker@inria.fr>:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Nautilus is a difficult beast, you all know. Even if you look close at the merge diff you may miss some important relation that only really visible during debugging. I did some test when reviewing this fix. But really, there are so many ways do you need to consider - different ways for opening nautilus - navigate with keys and mouse - selecting updating / changes from other browser and system changes. - refactorings It is really not that easy and I am happy that Franck and Yuri are actually trying to improve Nautilus. you can not easily tests all and make the changes and reviews bullet proof. nicolai
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr> wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org http://www.marcusdenker.de
In fact you couldnât spot the issue by just looking at the patch :) there was a very strange thing that could be fixed with one line, but to identify it Iâve spent probably 1.5 hours :) Uko
On 05 Sep 2015, at 19:03, Nicolai Hess <nicolaihess@web.de> wrote:
2015-09-05 11:48 GMT+02:00 Marcus Denker <marcus.denker@inria.fr <mailto:marcus.denker@inria.fr>>:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Nautilus is a difficult beast, you all know. Even if you look close at the merge diff you may miss some important relation that only really visible during debugging. I did some test when reviewing this fix. But really, there are so many ways do you need to consider - different ways for opening nautilus - navigate with keys and mouse - selecting updating / changes from other browser and system changes. - refactorings
It is really not that easy and I am happy that Franck and Yuri are actually trying to improve Nautilus. you can not easily tests all and make the changes and reviews bullet proof.
nicolai
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement... <https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...>
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
Just because looking at a patch isn't as powerful doesn't make it useless. There are still benefits * you are more aware of code changes * you can still spot conceptual problems * it's dead simple; the mail literally arrives at your doorsteps and all you need to do is look of course this isn't replacing proper review, but there is big still value in it. But even the "regular" process could be improved.
-> download the image before it was added
-> merge the slice.
For example I have startup script, that for images that have name "issuenumber - verify" on startup automatically opens MCBrowser on the slice so I can merge with two clicks. (It's not automerged because I want to test behavior before and after loading the slice.)
Since I'm mostly verifying my own fixes, I generally don't look at the changesets, but maybe you could autoopen a diff or something... Or even add a category to PharoLauncher "Pharo - To Review" so I wouldn't even need to type in the issue number. Peter
Le 5/9/15 20:46, Peter Uhnák a écrit :
Just because looking at a patch isn't as powerful doesn't make it useless. There are still benefits
* you are more aware of code changes * you can still spot conceptual problems * it's dead simple; the mail literally arrives at your doorsteps and all you need to do is look
of course this isn't replacing proper review, but there is big still value in it.
100% what I want to say. I will allocate time on a goodies that take a slice and produce a diff
But even the "regular" process could be improved.
-> download the image before it was added -> merge the slice.
For example I have startup script, that for images that have name "issuenumber - verify" on startup automatically opens MCBrowser on the slice so I can merge with two clicks. (It's not automerged because I want to test behavior before and after loading the slice.)
Since I'm mostly verifying my own fixes, I generally don't look at the changesets, but maybe you could autoopen a diff or something... Or even add a category to PharoLauncher "Pharo - To Review" so I wouldn't even need to type in the issue number.
Peter
Le 5/9/15 19:18, Yuriy Tymchuk a écrit :
In fact you couldnât spot the issue by just looking at the patch :) there was a very strange thing that could be fixed with one line, but to identify it Iâve spent probably 1.5 hours :)
sure this is not my point. my point is that we need to be able to know what changed. Right now I see a title and I do not have the time to click on each github link. I want to see diffs right in front of my nose. Stef
Uko
On 05 Sep 2015, at 19:03, Nicolai Hess <nicolaihess@web.de <mailto:nicolaihess@web.de>> wrote:
2015-09-05 11:48 GMT+02:00 Marcus Denker<marcus.denker@inria.fr <mailto:marcus.denker@inria.fr>>:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Nautilus is a difficult beast, you all know. Even if you look close at the merge diff you may miss some important relation that only really visible during debugging. I did some test when reviewing this fix. But really, there are so many ways do you need to consider - different ways for opening nautilus - navigate with keys and mouse - selecting updating / changes from other browser and system changes. - refactorings
It is really not that easy and I am happy that Franck and Yuri are actually trying to improve Nautilus. you can not easily tests all and make the changes and reviews bullet proof.
nicolai
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo<stepharo@free.fr <mailto:stepharo@free.fr>>wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
On 05 Sep 2015, at 21:34, stepharo <stepharo@free.fr> wrote:
Le 5/9/15 19:18, Yuriy Tymchuk a écrit :
In fact you couldnât spot the issue by just looking at the patch :) there was a very strange thing that could be fixed with one line, but to identify it Iâve spent probably 1.5 hours :)
sure this is not my point. my point is that we need to be able to know what changed. Right now I see a title and I do not have the time to click on each github link. I want to see diffs right in front of my nose.
I know very little about git, but the first thing that I tried worked: git clone https://github.com/pharo-project/pharo-core.git cd pharo-core/ git show 2536752ecdd88e15b00c6d3e29cb055d108731c8 I assume that we could put that textual information in the mail. In my terminal, it is coloured, but I still think that the github page is much better: https://github.com/pharo-project/pharo-core/commit/2536752ecdd88e15b00c6d3e2... (It happens that this commit changed quite a lot)
Stef
Uko
On 05 Sep 2015, at 19:03, Nicolai Hess <nicolaihess@web.de> wrote:
2015-09-05 11:48 GMT+02:00 Marcus Denker <marcus.denker@inria.fr>:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Nautilus is a difficult beast, you all know. Even if you look close at the merge diff you may miss some important relation that only really visible during debugging. I did some test when reviewing this fix. But really, there are so many ways do you need to consider - different ways for opening nautilus - navigate with keys and mouse - selecting updating / changes from other browser and system changes. - refactorings
It is really not that easy and I am happy that Franck and Yuri are actually trying to improve Nautilus. you can not easily tests all and make the changes and reviews bullet proof.
nicolai
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org http://www.marcusdenker.de
git clone https://github.com/pharo-project/pharo-core.git cd pharo-core/ git show 2536752ecdd88e15b00c6d3e29cb055d108731c8
But this is post-integration, we want to review things before they break something, not after. So ideally monkey reviews code on CI and if it passes the diff is sent from CI to the mailing list. Also these commits combine several unrelated issues, which is not good for review. Isn't what Squeak uses open-source and available somewhere? Can't we just reuse it? Peter
On 06 Sep 2015, at 01:41, Peter Uhnák <i.uhnak@gmail.com> wrote:
git clone https://github.com/pharo-project/pharo-core.git cd pharo-core/ git show 2536752ecdd88e15b00c6d3e29cb055d108731c8
But this is post-integration, we want to review things before they break something, not after. So ideally monkey reviews code on CI and if it passes the diff is sent from CI to the mailing list.
Also these commits combine several unrelated issues, which is not good for review.
Isn't what Squeak uses open-source and available somewhere? Can't we just reuse it?
Peter
You are right, Peter. But the way the rest of the world (and many people using git from Pharo or other Smalltalk implementations) is that you propose patches which I believe are quite similar to commits. That would be the equivalent of our slice, I guess. Sven
Peter I just want to get an idea of what is happening. Even after commit. Le 6/9/15 01:41, Peter Uhnák a écrit :
git clone https://github.com/pharo-project/pharo-core.git cd pharo-core/ git show 2536752ecdd88e15b00c6d3e29cb055d108731c8
But this is post-integration, we want to review things before they break something, not after. So ideally monkey reviews code on CI and if it passes the diff is sent from CI to the mailing list.
Also these commits combine several unrelated issues, which is not good for review.
Isn't what Squeak uses open-source and available somewhere? Can't we just reuse it?
Peter
On 6 Sep 2015 7:42 am, "Peter Uhnák" <i.uhnak@gmail.com> wrote:
git clone https://github.com/pharo-project/pharo-core.git cd pharo-core/ git show 2536752ecdd88e15b00c6d3e29cb055d108731c8
But this is post-integration, we want to review things before they break something, not after. So ideally monkey reviews code on CI and if it passes the diff is sent from CI to the mailing list.
Also these commits combine several unrelated issues, which is not good for review.
Isn't what Squeak uses open-source and available somewhere? Can't we just reuse it?
I thought the Squeak email diffs were post-commit?? Now what might be an interesting work flow would be when an issue is tagged Fix Review Needed and the CI monkey validates the issue it pushes the slice to a new IssueNNNNN branch in github and branch diff emailed from there. Cheers ben
You are right, Peter. But the way the rest of the world (and many people using git from Pharo or other Smalltalk implementations) is that you propose patches which I believe are quite similar to commits. That would be the equivalent of our slice, I guess.
I don't think that slice has equivalence in git, because we have changes much more fine-grained. Git patch is simply a diff between two different versions. And even though github (or bitbucket) offer things like "pull requests", they are only a nicer way of presenting a diff. But this doesn't really matter, because the result will be still the same. Now what might be an interesting work flow would be when an issue is tagged
Fix Review Needed and the CI monkey validates the issue it pushes the slice to a new IssueNNNNN branch in github and branch diff emailed from there.
Unless we want to migrate from fogzbugz to github and do code-reviews there in pull requests there's really no reason to create new branches. You can just create detached commits. (But then again, creating branches is dirt cheap). Actually for a diff you don't need to even commit, because you can compare your current code to the commited one (just like in Monticello). Peter
But this is all implementation details, I'm still not sure what exactly we (Stef) want to send. * change diff after each successful CI build for an issue? * diff after the change has been integrated? Peter
What I would like is to have all the changes after one integration with + and - showing the changes. I grouped a bit my analysis in https://pharo.fogbugz.com/f/cases/16491/Revisiting-TextDiffBuilder But now I should finish the other tasks I have pending. I should finish also some issues I started around announcers. Stef Le 6/9/15 15:50, Peter Uhnák a écrit :
But this is all implementation details, I'm still not sure what exactly we (Stef) want to send.
* change diff after each successful CI build for an issue? * diff after the change has been integrated?
Peter
Le 6/9/15 00:33, Sven Van Caekenberghe a écrit :
On 05 Sep 2015, at 21:34, stepharo <stepharo@free.fr> wrote:
Le 5/9/15 19:18, Yuriy Tymchuk a écrit :
In fact you couldnât spot the issue by just looking at the patch :) there was a very strange thing that could be fixed with one line, but to identify it Iâve spent probably 1.5 hours :) sure this is not my point. my point is that we need to be able to know what changed. Right now I see a title and I do not have the time to click on each github link. I want to see diffs right in front of my nose. I know very little about git, but the first thing that I tried worked:
git clone https://github.com/pharo-project/pharo-core.git cd pharo-core/ git show 2536752ecdd88e15b00c6d3e29cb055d108731c8
I went to github. I do not want to have to type the hash and this. How do I know that this this hash corresponds to the fixes that have been submitted? Then the output is ***awful***. So if this is that easy why can we have a mail with the diff? Stef
I assume that we could put that textual information in the mail.
In my terminal, it is coloured, but I still think that the github page is much better:
https://github.com/pharo-project/pharo-core/commit/2536752ecdd88e15b00c6d3e2...
(It happens that this commit changed quite a lot)
Stef
Uko
On 05 Sep 2015, at 19:03, Nicolai Hess <nicolaihess@web.de> wrote:
2015-09-05 11:48 GMT+02:00 Marcus Denker <marcus.denker@inria.fr>:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added -> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Nautilus is a difficult beast, you all know. Even if you look close at the merge diff you may miss some important relation that only really visible during debugging. I did some test when reviewing this fix. But really, there are so many ways do you need to consider - different ways for opening nautilus - navigate with keys and mouse - selecting updating / changes from other browser and system changes. - refactorings
It is really not that easy and I am happy that Franck and Yuri are actually trying to improve Nautilus. you can not easily tests all and make the changes and reviews bullet proof.
nicolai
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org http://www.marcusdenker.de
Hi, There is clearly no downside of having more detailed diff information available. We do not have it now, and the proposal of Stef is more than reasonable. It would be great to have someone willing to spend a bit of time helping him to achieve this. Who would like to give this a try? Cheers, Doru On Sun, Sep 6, 2015 at 10:28 AM, stepharo <stepharo@free.fr> wrote:
Le 6/9/15 00:33, Sven Van Caekenberghe a écrit :
On 05 Sep 2015, at 21:34, stepharo <stepharo@free.fr> wrote:
Le 5/9/15 19:18, Yuriy Tymchuk a écrit :
In fact you couldnât spot the issue by just looking at the patch :) there was a very strange thing that could be fixed with one line, but to identify it Iâve spent probably 1.5 hours :)
sure this is not my point. my point is that we need to be able to know what changed. Right now I see a title and I do not have the time to click on each github link. I want to see diffs right in front of my nose.
I know very little about git, but the first thing that I tried worked:
git clone https://github.com/pharo-project/pharo-core.git cd pharo-core/ git show 2536752ecdd88e15b00c6d3e29cb055d108731c8
I went to github. I do not want to have to type the hash and this. How do I know that this this hash corresponds to the fixes that have been submitted?
Then the output is ***awful***. So if this is that easy why can we have a mail with the diff?
Stef
I assume that we could put that textual information in the mail.
In my terminal, it is coloured, but I still think that the github page is much better:
https://github.com/pharo-project/pharo-core/commit/2536752ecdd88e15b00c6d3e2...
(It happens that this commit changed quite a lot)
Stef
Uko
On 05 Sep 2015, at 19:03, Nicolai Hess <nicolaihess@web.de> wrote:
2015-09-05 11:48 GMT+02:00 Marcus Denker <marcus.denker@inria.fr>:
On 05 Sep 2015, at 11:40, stepharo <stepharo@free.fr> wrote:
How can I see the changes? Our process is not good. Most of us do not get any chance understanding what is changing.
-> download the image before it was added
-> merge the slice.
Yes, our process is not good⦠but from a review perspective, this issue is the best we can do. *two* reviews, both from people actively contributing to exactly that part of the system.
If we require more, we will be back at a process where due to Fear we do nothing.
Nautilus is a difficult beast, you all know. Even if you look close at the merge diff you may miss some important relation that only really visible during debugging. I did some test when reviewing this fix. But really, there are so many ways do you need to consider - different ways for opening nautilus - navigate with keys and mouse - selecting updating / changes from other browser and system changes. - refactorings
It is really not that easy and I am happy that Franck and Yuri are actually trying to improve Nautilus. you can not easily tests all and make the changes and reviews bullet proof.
nicolai
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr> wrote: Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org http://www.marcusdenker.de
-- www.tudorgirba.com "Every thing has its own flow"
Oh yes I know. I'm not saying that looking at the changes we would have spotted the problems. I'm just saying that I want to get the changes in textual format in my mail-box as in squeak because it helps me understanding and knowing that something changed. I'm digging in the Diffbuilder and friends to see but I have some other tasks to finish first.
Nautilus is a difficult beast, you all know. Even if you look close at the merge diff you may miss some important relation that only really visible during debugging. I did some test when reviewing this fix. But really, there are so many ways do you need to consider - different ways for opening nautilus - navigate with keys and mouse - selecting updating / changes from other browser and system changes. - refactorings
It is really not that easy and I am happy that Franck and Yuri are actually trying to improve Nautilus. you can not easily tests all and make the changes and reviews bullet proof.
nicolai
Hi,
This is a side effect of
https://pharo.fogbugz.com/f/cases/16475/Nautilus-sends-too-many-announcement...
(which was reviewed by two people, so not obvious).
What happened is that #updatePackageGroupAndClassList calls itself via #selectedClass: leading to a loop.
Should be easy to fix for the people involved in case 16475.
Marcus
On Sat, Sep 5, 2015 at 8:51 AM, stepharo <stepharo@free.fr <mailto:stepharo@free.fr>> wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package.
Stef
-- -- Marcus Denker -- denker@acm.org <mailto:denker@acm.org> http://www.marcusdenker.de <http://www.marcusdenker.de/>
I found the issue. Fix almost ready. Should I reopen the case, or create a new one? Uko
On 05 Sep 2015, at 13:59, Stephan Eggermont <stephan@stack.nl> wrote:
On 05-09-15 08:51, stepharo wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package
I can move them to another package using drag-and-drop (issue 16488) in 50305
Stephan
As you wish. Thanks for your time. Now I will get really frustrated because I will not know simply what you did and I will have missed a good occasion to learn something. This is what sending the diff in text simply give us. Stef Le 5/9/15 14:09, Yuriy Tymchuk a écrit :
I found the issue. Fix almost ready. Should I reopen the case, or create a new one?
Uko
On 05 Sep 2015, at 13:59, Stephan Eggermont <stephan@stack.nl> wrote:
On 05-09-15 08:51, stepharo wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package I can move them to another package using drag-and-drop (issue 16488) in 50305
Stephan
I can give you a short explanation for the sake of more people knowing how Nautilus works. Regarding diffs and knowledge sharing, we should have a normal code review support with something like gerrit. Uko
On 05 Sep 2015, at 15:46, stepharo <stepharo@free.fr> wrote:
As you wish. Thanks for your time.
Now I will get really frustrated because I will not know simply what you did and I will have missed a good occasion to learn something. This is what sending the diff in text simply give us.
Stef
Le 5/9/15 14:09, Yuriy Tymchuk a écrit :
I found the issue. Fix almost ready. Should I reopen the case, or create a new one?
Uko
On 05 Sep 2015, at 13:59, Stephan Eggermont <stephan@stack.nl> wrote:
On 05-09-15 08:51, stepharo wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package I can move them to another package using drag-and-drop (issue 16488) in 50305
Stephan
Le 5/9/15 16:49, Yuriy Tymchuk a écrit :
I can give you a short explanation for the sake of more people knowing how Nautilus works. Yes please if you have some time. What I do not like is that we pass nil to indicate that there is no selection. I was thinking that an hierarchy of null objects could help but this is difficult to foresee if it will make sense. I also that all the logic update without reselecting is complex. I started to read the code and also move some condition about the model to the model class :). I would like to do just a simple pass reading all the code and not changing much to get now yet another better feel of nautilus. I will also remove the Nautilus help. I would like to rename the NautilusRefactoring because it is not about nautilus but about RB.
Regarding diffs and knowledge sharing, we should have a normal code review support with something like gerrit.
thomas was working on a code review tool and skip is taking over so I hope that we will have it.
Uko
On 05 Sep 2015, at 15:46, stepharo <stepharo@free.fr> wrote:
As you wish. Thanks for your time.
Now I will get really frustrated because I will not know simply what you did and I will have missed a good occasion to learn something. This is what sending the diff in text simply give us.
Stef
Le 5/9/15 14:09, Yuriy Tymchuk a écrit :
I found the issue. Fix almost ready. Should I reopen the case, or create a new one?
Uko
On 05 Sep 2015, at 13:59, Stephan Eggermont <stephan@stack.nl> wrote:
On 05-09-15 08:51, stepharo wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package I can move them to another package using drag-and-drop (issue 16488) in 50305
Stephan
Le 05/09/2015 21:39, stepharo a écrit :
Le 5/9/15 16:49, Yuriy Tymchuk a écrit :
I can give you a short explanation for the sake of more people knowing how Nautilus works. Yes please if you have some time. What I do not like is that we pass nil to indicate that there is no selection. I was thinking that an hierarchy of null objects could help but this is difficult to foresee if it will make sense. I also that all the logic update without reselecting is complex. I started to read the code and also move some condition about the model to the model class :). I would like to do just a simple pass reading all the code and not changing much to get now yet another better feel of nautilus. I will also remove the Nautilus help. I would like to rename the NautilusRefactoring because it is not about nautilus but about RB.
Regarding diffs and knowledge sharing, we should have a normal code review support with something like gerrit.
thomas was working on a code review tool and skip is taking over so I hope that we will have it.
Uko
I am also interesting for the informations about Nautilus :) Each time I try to check some code from it it's Hell! -- Cyril
Ok, so here it goes. There was a main method: #package:class:category:method: (https://github.com/pharo-project/pharo-core/blob/8a1f79b44b14f8eafaf6d91a8ba... <https://github.com/pharo-project/pharo-core/blob/8a1f79b44b14f8eafaf6d91a8ba...>). This method was setting a state of the model by sending #selectedPackage:, #selectedClass:, â¦â¦â¦ to self. Each of this 4 methods were announcing if the parameter was not nil. As you may guess #package:class:category:method: was used through the whole system with occasional nil params. I hate working with nils. So what I did was to break the method into 4: #package: #package:class: #package:class:protocol: #package:class:protocol:method: So now there is no nil passing anymore. E.g. if you select only a class, you send #package:class:. Then Iâve moved announcements from #selectedPackage:, #selectedClass:, â¦â¦â¦ to the newly created 4 methods. Also now announcements know the parent scope i.e. NautilusProtocolSelected besides protocol knows also about the class and package that played a role on the selection. This are essentially all the changes. The issue that occurred later (when selecting a package class was not deselected) is because #selectedClass: was doing `self selectedProtocol: nil`, same with protocol. But #selectedPackage: was not âdeselectingâ a class, probably because it was handled by calling `model package: SomePackage class: nil category: nil method: nil` in a first place.
On 05 Sep 2015, at 21:39, stepharo <stepharo@free.fr> wrote:
Le 5/9/15 16:49, Yuriy Tymchuk a écrit :
I can give you a short explanation for the sake of more people knowing how Nautilus works. Yes please if you have some time. What I do not like is that we pass nil to indicate that there is no selection. I was thinking that an hierarchy of null objects could help but this is difficult to foresee if it will make sense. I also that all the logic update without reselecting is complex. I started to read the code and also move some condition about the model to the model class :). I would like to do just a simple pass reading all the code and not changing much to get now yet another better feel of nautilus. I will also remove the Nautilus help. I would like to rename the NautilusRefactoring because it is not about nautilus but about RB.
Regarding diffs and knowledge sharing, we should have a normal code review support with something like gerrit.
thomas was working on a code review tool and skip is taking over so I hope that we will have it.
Uko
On 05 Sep 2015, at 15:46, stepharo <stepharo@free.fr> wrote:
As you wish. Thanks for your time.
Now I will get really frustrated because I will not know simply what you did and I will have missed a good occasion to learn something. This is what sending the diff in text simply give us.
Stef
Le 5/9/15 14:09, Yuriy Tymchuk a écrit :
I found the issue. Fix almost ready. Should I reopen the case, or create a new one?
Uko
On 05 Sep 2015, at 13:59, Stephan Eggermont <stephan@stack.nl> wrote:
On 05-09-15 08:51, stepharo wrote:
Hi
I do not know if this is linked to recent changes but we cannot remove classes or move them to another package I can move them to another package using drag-and-drop (issue 16488) in 50305
Stephan
participants (10)
-
Ben Coman -
Ferlicot D. Cyril -
Marcus Denker -
Nicolai Hess -
Peter Uhnák -
Stephan Eggermont -
stepharo -
Sven Van Caekenberghe -
Tudor Girba -
Yuriy Tymchuk