yackle / CLImageEditor

MIT License
2.22k stars 574 forks source link

Question: Is Menu Customisation for Tool Availability No Longer Working? #161

Closed hollowaykeanho closed 7 years ago

hollowaykeanho commented 7 years ago

Problem

Hello. Has anyone experiencing a similar problem that I have: the menu customization according to the README.md method is no longer working.


I was using the following code back in XCode7 and it was working, yielding only 3 tools. However, after migrating to XCode8 and some major changes, I can no longer disable the tool. These are the deltas:

Environment

  1. XCode Migration from version 7 to version 8.1
  2. Migrated code architecture from MVC to ReactiveCocoa MVVM.
  3. Development Environment: OSX El Capitan to OSX Sierra version 10.12.1, MacBook Pro, (Retina 13-inch 2015)
  4. CLImageEditor: version 0.1.6
  5. Installation: Cocoapods

The code snippet:

- (void)presentEditPageUI:(UIViewController *)controller
        processingHandler:(void(^)(id editorView))processingHandler
        completionHandler:(void(^)(void))completionHandler
{
    UIImage *page = [... // sourced image correctly];
    CLImageEditor *editor = [[CLImageEditor alloc] initWithImage:page];
    editor.delegate = (id)self;
     ...

    CLImageToolInfo *tool;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLFilterTool" recursive:NO];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLAdjustmentTool" recursive:NO];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLEffectTool" recursive:NO];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDrawTool" recursive:NO];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLSplashTool" recursive:NO];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLBlurTool" recursive:NO];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLStickerTool" recursive:NO];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLToneCurveTool" recursive:NO];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLEmoticonTool" recursive:NO];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLResizeTool" recursive:NO];
    tool.optionalInfo[@"presetSizes"] = @[@240, @320, @480, @640, @800, @960, @1024];
    tool.optionalInfo[@"limitSize"] = @1728;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLTextTool" recursive:NO];
    tool.available = NO;

    [controller presentViewController:editor animated:YES completion:nil];
}
yackle commented 7 years ago

I couldn't reproduce it with demo. Can I see the omitted part of your code?

Thanks.

hollowaykeanho commented 7 years ago

The omitted-part:

    self.storedCompletionHandler = completionHandler;

Update on my side:

Currently I'm working around the problem by supplying NO in each targeted tool:

+ (BOOL)isAvailable
{
   return NO;
   // return ([UIDevice iosVersion] >= 5.0);
}

This piece works for now.

yackle commented 7 years ago

Is not processingHandler used? Is it possible to tell me a small sample with this issue?

hollowaykeanho commented 7 years ago

Nope (EDIT: i mean processingHandler is not used). We can ignore processingHandler and completionHandler. Both are working fine.

(EDIT: by default, say CLFilterTool is set to not available, it will disappear from the menu, making it not available for user to access it. However, currently, it doesn't work anymore and always be accessible regardless the tool is set to NO).

Update on my side:

Apparently, the sub-tool is working fine but not the for main tool. I've tested on all the filters, and all of them disappeared. However, for the main "Filter" on the menu is maintaining its appearance. I have switched the recursive to YES instead of NO. Should I narrow the investigation to the framework source-code?

    // create the editor
    CLImageToolInfo *tool;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLFilterTool" recursive:YES];
    tool.available = NO;

    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultVignetteFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultEmptyFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultLinearFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultInstantFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultProcessFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultTransferFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultSepiaFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultChromeFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultFadeFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultCurveFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultTonalFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultNoirFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultMonoFilter" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDefaultInvertFilter" recursive:YES];
    tool.available = NO;

    tool = [editor.toolInfo subToolInfoWithToolName:@"CLAdjustmentTool" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLEffectTool" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLDrawTool" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLSplashTool" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLBlurTool" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLStickerTool" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLToneCurveTool" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLEmoticonTool" recursive:YES];
    tool.available = NO;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLResizeTool" recursive:YES];
    tool.optionalInfo[@"presetSizes"] = @[@240, @320, @480, @640, @800, @960, @1024];
    tool.optionalInfo[@"limitSize"] = @1728;
    tool = [editor.toolInfo subToolInfoWithToolName:@"CLTextTool" recursive:NO];
    tool.available = NO;

    [controller presentViewController:editor animated:YES completion:nil];
yackle commented 7 years ago

The reason of the issue is that the editor's viewDidLoad: is called before the tool settings. Does that ring a bell?

hollowaykeanho commented 7 years ago

Bug Found

In _CLImageEditorViewController.m, line 209, the [self setMenuView]; is executed right at viewDidLoad: instead of viewDidAppear:. By postponing it to viewDidAppear:, it works like before.

The current code:

- (void)viewWillAppear:(BOOL)animated
{
    [super viewWillAppear:animated];
    [self setMenuView];
    if(self.targetImageView){
        [self expropriateImageView];
    }
    else{
        [self refreshImageView];
    }
}

Will this change breaks the current demo project?

yackle commented 7 years ago

I agree. But that alone is not enough, we need to refactor setMenuView. I'll try.

hollowaykeanho commented 7 years ago

I've raised a pull request on fixing the current bug. We may proceed with the refactoring setMenuView.

yackle commented 7 years ago

I rename setMenuView to resetToolSettings and make it possible to call programally. Since viewWillAppear may be called many times, it has been restored to viewDidLoad.

So you need call resetToolSettings after tool settings. How do you think about these changes?

yackle commented 7 years ago

version 0.1.7 is available

hollowaykeanho commented 7 years ago

Superbly agreed with you. This is better than my fixes. I think it's better to use refreshToolSettings than resetToolSettings but it's low priority since it's about naming. reset seems to make me think my settings are not going to work.

Thanks for releasing the fix promptly! Love working with you.

yackle commented 7 years ago

OK, I followed your suggestion(185a134482dda0eba365a53e3551114daf88fe26). Thanks!