Page MenuHomePhabricator

elm_config: remove profile name reading logic from data dir
ClosedPublic

Authored by akanad on Nov 23 2018, 3:09 AM.

Details

Summary

Nothing writes any profile name on profile.cfg inside data dir
This patch removes the logic.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
akanad created this revision.Nov 23 2018, 3:09 AM
akanad requested review of this revision.Nov 23 2018, 3:09 AM

IMHO, this patch is obviously good to land, isn't it?
nowhere writes profile.cfg located on a data dir.

cedric accepted this revision.Nov 28 2018, 4:08 PM
This revision is now accepted and ready to land.Nov 28 2018, 4:08 PM
This revision was automatically updated to reflect the committed changes.
ManMower reopened this revision.Fri, Jan 11, 7:10 AM
ManMower added a subscriber: ManMower.

I think maybe we need to reconsider this, perhaps revert it?

The build install writes a profile in /usr/share/elementary or /usr/local/share/elementary and it's used as the default when there's no .elementary in a user's home dir?

It also seems it's a different default than when no config file is found at all - so with this patch a fresh install with no user home dir will end up with "thumb scrolling" (don't know what it is, but seems tragically broken) on by default.

So I think if this patch stays in we should stop installing a config in /usr/share/elementary and fix up the fallback to not enable thumb scroll?

The bug I'm seeing is that when I remove my ~/.elementary and run elementary_test's mirrored editor I can drag all the widgets around in the most bizarre way. That's just one example, lots of other stuff is broken. Seems like several bugs at play here, but none of them became visible to me until this patch landed.

This revision is now accepted and ready to land.Fri, Jan 11, 7:10 AM

@cedric @akanad I can confirm what Derek is saying. This patch removed the system fallback of the elm config - and thus left us with the code fallback which is not defaulted well :)

Besides that - the premise of this patch that "nothing writes to profile.cfg in elm's data directory" is false, or at the least misleading... A profile.cfg is installed to elm's data dir to be used as the default fallback when a new user uses elementary for the first time and has no config.

zmike added a subscriber: zmike.Fri, Jan 11, 7:20 AM

elm seems to suffer from the same issues that enlightenment does: the hardcoded config defaults are not the same as the defaults in the installed default config files.

This should likely be resolved in a more comprehensive manner wherein e.g., the installed config files should be generated based on the hardcoded defaults at build time.

Would it not be easiest to completely remove the system wide default files and only provide hardcoded defaults?

We can't discard hard coded defaults because we'd fail to start if the files weren't present, so discarding the fallback file in /usr/share seems to be an improvement in maintainability?

I've created T7620 because discussion here is going beyond the concerns of this differential to the larger issues it reveals...

this never should have even passed review. it's obviously wrong - the code path is used. a simple look at the for loop being removed would have shown that

zmike added a comment.Wed, Jan 16, 5:26 AM

this never should have even passed review. it's obviously wrong - the code path is used. a simple look at the for loop being removed would have shown that

It's good that we can look back and say such things now, but this patch seems like it has exposed flaws in the way that code reaches efl's master branch. D7351 was on phab for nearly a week with everyone (including you) on cc, but it still landed. It then went to the commit mailing list, and there were similarly no issues raised on that list. Now, nearly 2 months later, we are finally catching and fixing it. Do you have any suggestions for how we might improve our patch merging processes to reduce the likelihood of such occurrences?

my bad, I though that nothing touch profile.cfg on datadir.
I guess this patch should be reverted.

bu5hm4n closed this revision.Tue, Jan 22, 5:24 AM