Skip to content
New issue

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

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

Already on GitHub? # to your account

Custom path for ARM devices with limited space #21

Merged
merged 3 commits into from
Sep 14, 2019

Conversation

horstle
Copy link
Collaborator

@horstle horstle commented Apr 20, 2018

#20

@emilsvennesson
Copy link
Owner

I will review this before the week is over... sorry for taking so long.

@witzatom
Copy link

I could really use this feature.

@horstle horstle force-pushed the custom_path branch 2 times, most recently from d0f89e0 to 910529b Compare February 18, 2019 00:47
@dagwieers dagwieers added the bug label Aug 15, 2019
@dagwieers dagwieers modified the milestones: Future, 0.5.0 Aug 18, 2019
@dagwieers dagwieers added the arm Related to ARM architecture label Aug 19, 2019
@dagwieers
Copy link
Collaborator

Can you please rebase?

@horstle
Copy link
Collaborator Author

horstle commented Aug 19, 2019

Done.

@horstle horstle force-pushed the custom_path branch 2 times, most recently from 8fc4d77 to b2349f0 Compare August 19, 2019 21:10
@horstle horstle force-pushed the custom_path branch 3 times, most recently from 83eed36 to cc60536 Compare August 22, 2019 09:06
@horstle
Copy link
Collaborator Author

horstle commented Aug 22, 2019

The lint issues in Travis are no issues of this PR.
The first one (no-member) is just wrong, xbmcgui.Dialog().browseSingle() does exist.
The second is simply not created by this PR, as it doesn't change the corresponding lines.

See next comment.

@horstle horstle force-pushed the custom_path branch 7 times, most recently from 2690f5a to 55b4593 Compare August 23, 2019 14:46
@horstle
Copy link
Collaborator Author

horstle commented Aug 23, 2019

Alright, all pylint issues are gone, but the too-many-lines and too-many-statements (at _install_widevine_arm()) are simply suppressed and should be addressed in the future.

The first argument of xbmcgui.Dialog().browseSingle() is actually called "type" in the documentation, but since "type" already exists, I had to rename it (to "atype"). I don't think this could create an issue, because from what I understand if somebody ever called browseSingle() with a keyword-argument type= he would redefine the builtin.

@mediaminister
Copy link
Collaborator

@horstle While testing your pull request I came to the conclusion that there is no option to use the default temporary directory again when a custom directory was set once.
Setting the default temp directory as default in the settings.xml fixes this, so there is no need any more to call this setting "custom". We can rely completely on settings.xml to define the temp directory.

I'll add a commit to this pull request which implements this.

@mediaminister
Copy link
Collaborator

This should be ready to merge now. Please test this first on ARM devices.

@horstle horstle force-pushed the custom_path branch 2 times, most recently from 7fada2f to 10e3881 Compare September 13, 2019 20:54
Copy link
Collaborator

@dagwieers dagwieers left a comment

Choose a reason for hiding this comment

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

Looking good. Just a few remarks, nothing critical.

resources/settings.xml Outdated Show resolved Hide resolved
resources/settings.xml Outdated Show resolved Hide resolved
test/xbmc.py Outdated Show resolved Hide resolved
test/xbmcgui.py Outdated Show resolved Hide resolved
@dagwieers
Copy link
Collaborator

I think I fixed the coverage reporting now. Thanks for your patience!
If you would rebase your branch (git pull --rebase origin master) and force-push, I am confident all goes green (not that it really matters, but would be a nice test).

Add possibility to change temporary path on ARM devices if the ChromeOS image is too big.

Remember custom path (as setting), check specified custom path whether
there is enough space.
@dagwieers
Copy link
Collaborator

dagwieers commented Sep 14, 2019

@horstle I did the rebase and pushed one commit with the changes I proposed here.

This commit includes:

  • Change temp_dir info temp_path
  • Use special://masterprofile instead of special://profile
  • Fix disabled setting
  • Use standard argument name for xbmcgui.browseSingle
  • Rename 'Temporary directory' into 'Temporary download directory'
  • Translation to Dutch

Please do not force-push :-)

@dagwieers
Copy link
Collaborator

I think we may want to reorganize the settings at some point.
Probably add a seperator, and rename 'Troubleshooting' into 'Expert'.

Copy link
Collaborator

@dagwieers dagwieers left a comment

Choose a reason for hiding this comment

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

For me this is ready to be merged. Even if we decide to make this hidden for non-ARM platforms later.

This commit includes:
- Change `temp_dir` info `temp_path`
- Use `special://masterprofile` instead of `special://profile`
- Fix `disabled` setting
- Use standard argument name for xbmcgui.browseSingle
- Rename 'Temporary directory' into 'Temporary download directory'
@mediaminister
Copy link
Collaborator

If people with ARM devices can confirm this works in real life, I'm okay with merging this.

@dagwieers
Copy link
Collaborator

I have tested this, but not with a device that hasn't sufficient disk space.

@dagwieers dagwieers merged commit adb66aa into emilsvennesson:master Sep 14, 2019
@dagwieers dagwieers modified the milestones: v0.5.0, v0.4.3 Sep 14, 2019
@horstle
Copy link
Collaborator Author

horstle commented Sep 15, 2019

Great to see this finally merged, thanks! I really hope I didn't break anything with the last changes.

@horstle horstle deleted the custom_path branch September 15, 2019 19:04
@dagwieers
Copy link
Collaborator

We still have time for testing before we release. It would be nice to test the following cases:

  • Test with insufficient disk space
  • Point to a new temporary path (potentially on other storage)
  • Test with second storage that also does not have insufficient disk space
  • Remove second storage with temporary path still pointing to it
  • Test with non-existing temporary path

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
arm Related to ARM architecture bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants