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

OOP implementation and improve preferences UX #14

Merged
merged 12 commits into from
Jul 21, 2021
Merged

Conversation

thanhph111
Copy link
Collaborator

@thanhph111 thanhph111 commented Jul 18, 2021

Proposed changes

This pull request proposes an OOP approach to the clipboard implementation, which is better for maintenance and development.
These changes also aim to improve user experience on the add-on preferences. Details:

  • Reorganize code to specialized modules follow Separation of concerns design : preferences.py (add-on preferences, menu UI, keymaps) and operators.py (operator classes only). __init__.py now contains only metadata (7dc3cc5).
  • Handle platform operations related to clipboards with following classes which are concreted classes of Clipboard class (support by a list of classes described in dd9c494).
  • Rewrite all the operator to perform those classes, reduce duplicate code, better report and better context selection.
  • Images should be saved in Blender temporary folder (will be deleted when closing session) if .blend file is not available. This option will free user from specifying a default directory. However, we should allow users to overwrite this directory.
    The new preferences below (2a99d8c) added:
    • An option to use another directory for images instead of the temporary directory, including another option to force use it.
    • An option to use subdirectory in the .blend file path.
    • An option to disable debug message.
Old preferences New preferences
Screenshot from 2021-07-21 15-01-16 Screenshot from 2021-07-21 15-02-10
  • Discuss about the idea: moving scattered images to .blend file folder when saving. (out of the PR scope)

Further comments

  • Windows operations are still very slow. I think it's because the current PowerShell approach depends on too much operations before touching Windows API.
  • When rewrite module linux.py, fix an issue make Linux clipboard cannot copy an JPG image (or other types except PNG) (dd9c494).

Also add 'Documentation' button to the add-on preferences menu and
rewrite `bl_info`.
- Free `__init__.py` from a bunch of processing code, keeping concern on
  metadata only. Now all code for preferences, menus and keymaps belongs
  to `preferences.py`; `operators.py` is responsible for all operators.
- Every module is responsible for registering and unregistering its
  components from now.
- Push `bl_info` to the top of the file, so we have to ignore E402 of
  'Flake8' from `__init__.py`.
- Implement first OOP way to handle clipboard for `LinuxClipboard`. Some
  classes are used to support:
  - `Clipboard`: an abstract class to handle the clipboard.
  - `Process`: a class to handle command line process with safe
    `Popen.communicate()`.
  - `Report`: a class to generate a report for operations status.
  - `Image`: a class to hold image information.
- Also rewrite all the operators to fit the new OOP way.
- Add `poll()` method to all operators to let them execute on the right
  context.
- Default directory currently takes `.blend` files (if possible) or
  'Blender' temporary folder `bpy.app.tempdir`, it will be deleted after
  closing session, user won't have to specify it anymore. Improve this
  to be more flexible later.
- Most operations now explicitly inform users about its status, include
  logging on the console for debugging purpose.
- Add docstring to most of the classes and functions.
- Currently disable 'Windows' and 'Darwin' clipboard support.
- Also fix a bug that make old implementation cannot copy a JPG image
  (or other types except PNG) from the Image Editor to the clipboard
  with guesstimated MIME types.
- Implement the `WindowsClipboard` class following OOP using old
  approach, still really slow.
- Fix the missing f-string sign in `LinuxClipboard.push()`.
- Turn `__str__` into `__repr__` for `Report`.
- Add `split` options for output of a process.
- Reinforce the context in `poll()` for all operators. Allow operators
  related to Image Editor run on both Image Editor and UV Editor.
  Attention: 'Python' API changes the `bpy.area.ui_type` of Image Editor
  from `VIEW` ('Blender' 2.83) to `IMAGE_EDITOR` ('Blender' 2.93). Both
  areas still have the same `bpy.area.type` value (`IMAGE_EDITOR`).
- Rewrite all operators docstring to be more friendly on 'Blender' UI
  because they're used to display hover tooltips.
- Prioritize using `from ... import ...` more than `import ...` in
  `image` module.
Using previous approach with `osascript`. Haven't tested yet, to be
reviewed in #14.
@thanhph111
Copy link
Collaborator Author

@williamchange I am implementing the OOP version of macOS clipboard based on your code, but don't have any knowledge to do the test. Can you help me with this?

@thanhph111 thanhph111 changed the title OOP implementation OOP implementation and improve preferences UX Jul 19, 2021
This will add an 'Report a Bug' button to the add-on preferences.
The docstrings for operators should be short, no period and contain no
context as they will be display under UI.
- `IMAGEPASTE_OT_imageeditor_copy` operator should check if loaded image
  is not an empty render result.
- `IMAGEPASTE_OT_shadereditor_paste` operator should check if a material
  is loading.
- Reference: "We often put imports within functions body which is not
  pep8 compliant. This is done to speed up Blender's startup times to
  avoid loading in many files and libraries which the user may never
  access." (https://wiki.blender.org/wiki/Style_Guide/Python).
- Also improve condition checking accuracy.
- Also remove `universal_newlines` argument from `subprocess.Popen` as
  it's an alias for `text` argument.
@williamchange
Copy link
Contributor

I just checked it out, fixed a couple of issues and opened a PR for it. But there's still some other issues.

- Add new preferences UI with:
  - An option to use another directory for images instead of the
    temporary directory, including another option to force use it.
  - An option to use subdirectory in the `.blend` file path.
  - An option to disable debug message.
- Split multiple utilities code to to `helper.py` module.
- Rewrite the `get_save_directory()` function to adapt with the new
  preferences and fix the "not existent directory" bug.
- Add missing execute function call and correctly capturing `stdout`.
- Pass a report when an image is written with a warning (catch by
  checking the saved file instead of `stderr`). 
- Use `pasteboard` to check for filepaths.
- Target TIFF when checking clipboard contents via `pasteboard`.
@thanhph111 thanhph111 requested a review from b-init July 21, 2021 08:14
@thanhph111 thanhph111 marked this pull request as ready for review July 21, 2021 08:14
@thanhph111
Copy link
Collaborator Author

@Yeetus3141 I suppose we did a good refactoring here, most changes are under the hood but it will help us a lot in the future. We also have a new preferences menu which saves users from having to specify a saving directory if they don't care. About the feature that moves scattered pasted images into .blend file directory when users save file, I think we should save for another PR. Is there any changes you think we should add to this?

@b-init
Copy link
Owner

b-init commented Jul 21, 2021

Just tried the new build. Seems to be great! Awesome work you guys.
And yep the feature of moving scattered images would be good as another PR.
And I'll be working on integrating the Video Editor features into the new structure, with possible additions. Unless you've been progressing on it already?

@thanhph111
Copy link
Collaborator Author

Yeah, don't worry, I plan to implement right after merging this and will request your review then.

@thanhph111 thanhph111 merged commit d39cbe0 into main Jul 21, 2021
thanhph111 added a commit that referenced this pull request Jul 21, 2021
Using previous approach with `osascript`. Haven't tested yet, to be
reviewed in #14.
@thanhph111 thanhph111 deleted the oop-implementation branch July 21, 2021 09:38
@thanhph111 thanhph111 restored the oop-implementation branch July 22, 2021 17:19
@thanhph111 thanhph111 deleted the oop-implementation branch July 22, 2021 17:28
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.

3 participants