Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Conversation

tokejepsen
Copy link
Member

@tokejepsen tokejepsen commented Aug 17, 2020

Unfortunately the nature of Loaders is that they operate on one representation at a time, so when multiple subsets are selected the settings dialog opens multiple times.
This could be solved with Loader options.

I was also thinking of splitting out the calculate to a different Loader, so better separate destructive actions.

@tokejepsen tokejepsen added the type: enhancement Enhancements to existing functionality label Aug 17, 2020
@tokejepsen tokejepsen self-assigned this Aug 17, 2020
@tokejepsen
Copy link
Member Author

Resolves #396

@mkolar
Copy link
Member

mkolar commented Sep 15, 2020

This is crashing with our latest avalon core, but that is a bug in the core itself. @iLLiCiTiT could you please have a look?
image

@tokejepsen
Copy link
Member Author

@mkolar the fix is here; ynput/avalon-core#176

@mkolar
Copy link
Member

mkolar commented Sep 16, 2020

Ah yeah. Sorry my bad

Copy link
Member

@mkolar mkolar left a comment

Choose a reason for hiding this comment

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

It's working pretty well for a first iteration of the action. there are a few caveats. Currently it only prints the calculated filesize if the debug mode is on, because it only prints to console. Secondly after deleting the library loader crashes when clicked :). @iLLiCiTiT will be able to advise about that.

@iLLiCiTiT
Copy link
Member

Secondly after deleting the library loader crashes when clicked :)

I think the easiest would be to trigger app.refresh() when delete part is done.

But I'm not sure if that will trigger refresh of subsets widget. If not then must be executed app.data["widgets"]["subsets"].model.refresh().

@tokejepsen
Copy link
Member Author

I think the easiest would be to trigger app.refresh() when delete part is done.

Cool, I can have a look at that later.

@iLLiCiTiT any thoughts on implementing Loader settings into the Library Loader?

@iLLiCiTiT
Copy link
Member

@iLLiCiTiT any thoughts on implementing Loader settings into the Library Loader?

I thought they are there? Created PR ynput/avalon-core#192

@mkolar
Copy link
Member

mkolar commented Sep 22, 2020

Loader options in library are now merged to 2.x/develop

@tokejepsen
Copy link
Member Author

Loader options in library are now merged to 2.x/develop

Thanks. Added an item to check on the description.

@mkolar mkolar force-pushed the 2.x/develop branch 2 times, most recently from 46534d7 to 461dc0e Compare December 8, 2020 16:58
Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

./pype/tools/pyblish_pype/model.py:965:34: B008: Do not perform function call...
./pype/tools/pyblish_pype/model.py:965:34: B008: Do not perform function calls in argument defaults.  The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call.  If this is intended, assign the function call to a module-level variable and use that variable as a default value.
./pype/tools/pyblish_pype/model.py:969:31: B008: Do not perform function calls in argument defaults.  The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call.  If this is intended, assign the function call to a module-level variable and use that variable as a default value.
./pype/tools/pyblish_pype/model.py:994:41: B008: Do not perform function calls in argument defaults.  The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call.  If this is intended, assign the function call to a module-level variable and use that variable as a default value.
Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 11, in 
    sys.exit(main())
  File "/home/linters/.local/lib/python3.6/site-packages/flake8/main/cli.py", line 18, in main
    app.run(argv)
  File "/home/linters/.local/lib/python3.6/site-packages/flake8/main/application.py", line 393, in run
    self._run(argv)
  File "/home/linters/.local/lib/python3.6/site-packages/flake8/main/application.py", line 381, in _run
    self.run_checks()
  File "/home/linters/.local/lib/python3.6/site-packages/flake8/main/application.py", line 300, in run_checks
    self.file_checker_manager.run()
  File "/home/linters/.local/lib/python3.6/site-packages/flake8/checker.py", line 331, in run
    self.run_serial()
  File "/home/linters/.local/lib/python3.6/site-packages/flake8/checker.py", line 315, in run_serial
    checker.run_checks()
  File "/home/linters/.local/lib/python3.6/site-packages/flake8/checker.py", line 598, in run_checks
    self.run_ast_checks()
  File "/home/linters/.local/lib/python3.6/site-packages/flake8/checker.py", line 502, in run_ast_checks
    for (line_number, offset, text, check) in runner:
  File "/home/linters/.local/lib/python3.6/site-packages/flake8_django/checker.py", line 56, in run
    parser.visit(self.tree)
  File "/usr/lib/python3.6/ast.py", line 253, in visit
    return visitor(node)
  File "/usr/lib/python3.6/ast.py", line 261, in generic_visit
    self.visit(item)
  File "/home/linters/.local/lib/python3.6/site-packages/flake8_django/checker.py", line 39, in visit_ClassDef
    self.capture_issues_visitor('ClassDef', node)
  File "/home/linters/.local/lib/python3.6/site-packages/flake8_django/checker.py", line 33, in capture_issues_visitor
    self.generic_visit(node)
  File "/usr/lib/python3.6/ast.py", line 263, in generic_visit
    self.visit(value)
  File "/home/linters/.local/lib/python3.6/site-packages/flake8_django/checker.py", line 36, in visit_Call
    self.capture_issues_visitor('Call', node)
  File "/home/linters/.local/lib/python3.6/site-packages/flake8_django/checker.py", line 30, in capture_issues_visitor
    issues = checker.run(node)
  File "/home/linters/.local/lib/python3.6/site-packages/flake8_django/checkers/render.py", line 22, in run
    if isinstance(arg, ast.Call) and arg.func.id == 'locals':
AttributeError: 'Attribute' object has no attribute 'id'

@tokejepsen tokejepsen force-pushed the 2.x/feature/library_delete_old_versions branch from e77bdb0 to b09b419 Compare January 30, 2021 15:46
- Replaced settings dialog with Loader options.
- Replaced print statements with logging and message boxes.
@hound hound bot deleted a comment from tokejepsen Jan 30, 2021
@tokejepsen
Copy link
Member Author

There is currently an issue when you run this on multiple assets, where the size is calculated per asset rather than a total of all the assets. This is because the Loader action is run per asset rather than on all the assets.

Guessing this is a design implementation of the Loader we'll have to live with?

@mkolar
Copy link
Member

mkolar commented Feb 11, 2021

Guessing this is a design implementation of the Loader we'll have to live with?

I'm afraid so, at least for now. But I'm trying it out and it's looking good.

@tokejepsen
Copy link
Member Author

Guessing this is a design implementation of the Loader we'll have to live with?

I was thinking whether there was a Loader class or modify the existing Loader class , so the Loader gets passed a list of assets instead of a single one?

@tokejepsen
Copy link
Member Author

tokejepsen commented Feb 11, 2021

Actually since the context is being passed to the Loader, the context just needs another key assets for a list. But that the Loader logic would run it multiple times...


options = [
qargparse.Integer(
"versions", default=2, min=1, help="Versions to keep:"
Copy link
Member

Choose a reason for hiding this comment

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

we should allow minimum to 0, which effectively deletes the full version.

Copy link
Member

Choose a reason for hiding this comment

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

however in that case, the subset should be deleted with it.

qargparse.Integer(
"versions", default=2, min=1, help="Versions to keep:"
),
qargparse.Boolean("publish", help="Remove publish folder:")
Copy link
Member

Choose a reason for hiding this comment

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

To keep this clear to the user I'd change the attribute names to nicer versions to keep and delete folder. I know the behaviour and caught myself mistaking whether it's version to keep or delete multiple time during testing.

@mkolar
Copy link
Member

mkolar commented Feb 11, 2021

Actually since the context is being passed to the Loader, the context just needs another key assets for a list. But that the Loader logic would run it multiple times..

this kind of ties to the fact, that you see it on multiple representations, but it's really a version level action, so it's confusing. Maybe it could be fixed at the same time. @iLLiCiTiT what do you think?

image

@mkolar mkolar added this to the 2.15.0 milestone Feb 12, 2021
@mkolar mkolar modified the milestones: 2.15.0, 2.15.1 Feb 12, 2021
@mkolar mkolar changed the base branch from 2.x/develop to feature/delete_version_loader_action February 12, 2021 12:10
@mkolar
Copy link
Member

mkolar commented Feb 12, 2021

Happy to merge to an intermediate branch, for some final touches on our end.

@mkolar mkolar merged commit 4f61296 into ynput:feature/delete_version_loader_action Feb 12, 2021
@tokejepsen
Copy link
Member Author

Up to you guys. I'm happy to keep this PR open until we have something that is more production ready, ei. not having pops up per asset.

iLLiCiTiT pushed a commit that referenced this pull request Feb 12, 2021
@tokejepsen tokejepsen deleted the 2.x/feature/library_delete_old_versions branch March 13, 2021 08:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement Enhancements to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants