Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] main: Fix widget settings directory path to clear #3932

Merged
merged 4 commits into from
Jul 17, 2019

Conversation

ales-erjavec
Copy link
Contributor

Issue

Fixes gh-3931

Since gh-3772 the environ.widget_settings_dir is no longer the authoritative directory path.

Description of changes
  • Deprecate Orange.misc.environ.widget_settings_dir, (but make it return the correct path).
  • Change the application name to 'Orange' to preserve historical directory layout (the application name is used as the path component in the settings path).
Includes
  • Code changes
  • Tests
  • Documentation

Since biolabgh-3772 the `environ.widget_settings_dir` is no longer the
authoritative directory path.

Change the application name to 'Orange' to preserve historical
directory layout (the application name is used as the path component
in the settings path).
@codecov
Copy link

codecov bot commented Jul 10, 2019

Codecov Report

Merging #3932 into master will increase coverage by 0.26%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master    #3932      +/-   ##
==========================================
+ Coverage   84.71%   84.98%   +0.26%     
==========================================
  Files         371      376       +5     
  Lines       64929    66461    +1532     
==========================================
+ Hits        55006    56480    +1474     
- Misses       9923     9981      +58

@lanzagar
Copy link
Contributor

lanzagar commented Jul 12, 2019

I am still a bit confused. Which is the correct one to use (and why aren't all others deprecated)?
Edit: Sorry, most of the others are deprecated after the new PRs. So only the second remains strange... (and that is the one reported to be used in the deprecation of the third).

>>> Orange.version.version
'3.23.0.dev0+15d4fe6'
>>> Orange.canvas.config.widget_settings_dir()
'/home/lan/.local/share/Orange/3.23.0.dev/widgets'
>>> Orange.widgets.settings.widget_settings_dir()  # Wrong version?
'/home/lan/.local/share/Orange/0.0.0/widgets'
>>> Orange.misc.environ.widget_settings_dir()  # Now deprecated
'/home/lan/.local/share/Orange/3.23.0.dev/widgets'
>>> Orange.canvas.utils.environ.widget_settings_dir  # str not function! Module deprecated
'/home/lan/.local/share/Orange/3.23.0.dev/widgets'

return os.path.join(data_dir(versioned=versioned), "widgets")
warnings.warn(
f"'{__name__}.widget_settings_dir' is deprecated. "
"Use 'Orange.widgets.settings.widget_settings_dir'",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 'Orange.canvas.config.widget_settings_dir'?
And 'Orange.widgets.settings.widget_settings_dir' removed since it is wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orange.widgets.settings.widget_settings_dir is the effective directory (technically it is orangewidget.settings.widget_settings_dir. It will become the correct path once the application starts and configures.

>>> Orange.widgets.settings.widget_settings_dir()
'/Users/aleserjavec/Library/Application Support/Orange/0.0.0/widgets'
>>> Orange.canvas.config.Config().init()
'/Users/aleserjavec/Library/Application Support/Orange/3.23.0.dev/widgets'

This actually has to happen very soon before the widgets are imported.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so after configuration they both return the same correct path. But still, when should one use which
Orange.canvas.config.widget_settings_dir
Orange.widgets.settings.widget_settings_dir
and could we have just one?

Currently they also have different calls to orangewidget:
orangewidget.workflow.config.widget_settings_dir
orangewidget.settings.widget_settings_dir
I think the public API of orangewidget should have one path from which to import widget_settings_dir.
If the same thing is available from multiple locations (either in orangewidget or in orange) it just creates confusion and provides opportunities for problems in the future IMO.

@lanzagar lanzagar merged commit 7c4c4b5 into biolab:master Jul 17, 2019
@lanzagar lanzagar mentioned this pull request Sep 13, 2019
3 tasks
@ales-erjavec ales-erjavec deleted the fixes/clear-widgets-settings-dir branch March 16, 2020 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reset widget settings not working
2 participants