Page MenuHomePhabricator

elementary: improve lifecycle of model object in the fileselector widget.
AcceptedPublic

Authored by cedric on Wed, Mar 13, 2:59 PM.

Details

Summary

This is a minimal change and it would be best to refactor the code completely
using all the infrastructure we have now instead of the organically grown code,
but I am afraid of doing such a big change at this point of our release cycle.

Part of the improvement are use of efl_replace to make sure Eo object reference
are set to NULL once reference are dropped. Handling the case when a processed
child is actually pointing to an error. Also it is not supported by model to
get their parent stolen, so this has been fixed too. Finally setting the path
asynchronously was creating more trouble than needed, when it could be done in
a synchronous way.

Depends on D8335

Diff Detail

Repository
rEFL core/efl
Branch
devs/cedric/fileselector
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 10511
cedric created this revision.Wed, Mar 13, 2:59 PM
cedric requested review of this revision.Wed, Mar 13, 2:59 PM
cedric updated this revision to Diff 20594.Thu, Mar 14, 2:28 PM
cedric edited the summary of this revision. (Show Details)

Rebase.

cedric updated this revision to Diff 20648.Fri, Mar 15, 4:59 PM
cedric added a reviewer: YOhoho.

Rebase.

I don't understand this commit at all, can you describe a little bit more in the commit message what this here does ?

cedric updated this revision to Diff 20810.Wed, Mar 20, 11:35 AM
cedric edited the summary of this revision. (Show Details)

Rebase and correct.

bu5hm4n accepted this revision.Thu, Mar 21, 3:09 AM
This revision is now accepted and ready to land.Thu, Mar 21, 3:09 AM
bu5hm4n requested changes to this revision.Thu, Mar 21, 3:17 AM

#1 Open a large directory in fileselector, close the window -> SEGV
#2 Open a large directory in fileselector, click on home -> things get added to the view that are not in the directory that is least recently opened
#3 Open a fileselector, click arround -> observe a giant amount of errors in the console

This revision now requires changes to proceed.Thu, Mar 21, 3:17 AM

Okay, if i am now using the most upperst revision then the items do not get mixed up anymore. However, the first population run finishes before the new path is populated. This is something that I would not expect as a user.

Also, could you squash the this fixing change into this revision or place it before? Otherwise we would end up with a range of commits which are known to introduce bugs, i would love to not do that...

Okay, if i am now using the most upperst revision then the items do not get mixed up anymore. However, the first population run finishes before the new path is populated. This is something that I would not expect as a user.

I am not sure what you are saying, but will try to reproduce it.

Also, could you squash the this fixing change into this revision or place it before? Otherwise we would end up with a range of commits which are known to introduce bugs, i would love to not do that...

Except they don't fix exactly the same problem. The other patch are fixing bugs that are different from what this patches fixes. This patches doesn't introduce bugs, but make other bugs more visible. Arguably, to me, before this entire series, fileselector is not working. So I could merge all patch at once if the point was to have a smaller range of commits.

Given that we are heading from one *deepshit* issue into the next, i will just closed eye approve this, given that the appearance before the commits is the same just with a lots more errors.

Additionally, can you please try to sort your commits a little bit more after concerns ? Its close to impossible to review this amount of commits, where problems in 1 commits are answered with more commits on top. I really don't know if i will again review such a stack of things...

bu5hm4n accepted this revision.Mon, Mar 25, 3:23 AM
This revision is now accepted and ready to land.Mon, Mar 25, 3:23 AM

Additionally, can you please try to sort your commits a little bit more after concerns ? Its close to impossible to review this amount of commits, where problems in 1 commits are answered with more commits on top. I really don't know if i will again review such a stack of things...

Fileselector was pretty broken and had a tons of different issues. Some people like @zmike had there test failing frequently and I could not get mine to break. Eventually after fixing other issues, I think I get to the point where I could see his problems and fixed it. At no point in this series is there a bug introduced that is fixed by a later patch. It is just that you might have been in a configuration where you didn't see it before.

Fileselector was pretty broken and had a tons of different issues. Some people like @zmike had there test failing frequently and I could not get mine to break. Eventually after fixing other issues, I think I get to the point where I could see his problems and fixed it. At no point in this series is there a bug introduced that is fixed by a later patch. It is just that you might have been in a configuration where you didn't see it before.

I am not saying that things in general got worse, but the eo pathes don't looks related to this, they could have been already landed, most of the eio stuff looks like cleanup commits, which are surly helpfull for fixing the fileselector mess, but they could be landed before.

And yes, fileselector is a complete mess, but right now we still have 2 remaining thing open, which is the reason why i don't want to land this right now in the state that it is ...

I am not saying that things in general got worse, but the eo pathes don't looks related to this, they could have been already landed, most of the eio stuff looks like cleanup commits, which are surly helpfull for fixing the fileselector mess, but they could be landed before.

The eo path change is due to a recent change I made in Efl Model to ensure child Model lifecycle and didn't properly fix it here. This patch is mainly about adapting to that cleaner/defined behavior.

And yes, fileselector is a complete mess, but right now we still have 2 remaining thing open, which is the reason why i don't want to land this right now in the state that it is ...

Hum, can you point me to them? I am at the moment unable to reproduce any issue on my laptop.

Testsuite not passing: https://phab.enlightenment.org/D8450#152862
Open a large directory in a fs (like /usr/lib), click on home, notice that home will not be opened until the giant directory is finally there.