Fmstrat / wintile

Windows 10 window tiling for GNOME
GNU General Public License v3.0
424 stars 54 forks source link

Limit use of global scope and depricate Tweener #102

Closed Fmstrat closed 1 year ago

Fmstrat commented 1 year ago

From GNOME Extensions review. This was feedback on the diff between v7 and v8: https://github.com/Fmstrat/wintile/compare/v7...v8

What spare time I've had I've been focusing on samba-domain, so if @rsxchin or @phavekes have time, please let me know here you're jumping in.

1. You cannot create objects in global scope which is the same as init:
    - line 43-52 extension.js
    - line 16-24 prefs.js
    https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization

2. That's too much for global scope:

    - line 66 extension.js
    - line 69 extension.js
    - line 726-730 extension.js

    https://gjs.guide/extensions/review-guidelines/review-guidelines.html#only-use-init-for-initialization

3. Remove timeout on disable (line 643, 651, 674, 694, 859, 898 extension.js):
    https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources

4. `Tweener` is a deprecated module. Please remove it for the next version.
    You can use `ease()` instead. For example:

    ```js
    widget.ease({    
        x: newX,
        y: 10,
        opacity: 100,
        duration: 2000,
        mode: Clutter.AnimationMode.EASE_OUT_BOUNCE,
        onComplete: () => {
            log('Animation is finished');
        }
    });
  1. Remove line 926 (extension.js) if condition. You should disconnect those signals without being dependent on other condition. Also null out those signal id holders in disable (onWindowGrabBegin and onWindowGrabEnd).

If you need any help with your extension you can ask us on:

Related to: https://github.com/Fmstrat/wintile/issues/99

GrylledCheez commented 1 year ago

@Fmstrat, I'd like to help out. We can turn this into a checklist and knock it out for sure

GrylledCheez commented 1 year ago

1 and 2. #106

  1. 105

  2. 108

  3. 104

GrylledCheez commented 1 year ago

All items have been completed and merged. They work on my end.