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

Find - Add a Find command to Simple Comic based on recognized text. #84

Merged
merged 22 commits into from
Aug 1, 2022
Merged

Find - Add a Find command to Simple Comic based on recognized text. #84

merged 22 commits into from
Aug 1, 2022

Conversation

DavidPhillipOster
Copy link

No description provided.

@DavidPhillipOster
Copy link
Author

I'm happy to answer any questions you may have about this pull request.

In short: Apple's NSTextFinder is a standard U.I. for a find command, but I could not use it because it expects that the app already has a complete text document. SimpleComic just has the text for the current page. So, I re-implemented the U.I. in OCRVision/OCRFindViewController, which talks to the actual Find engine OCRVision/OCRFind. The actual work of doing the Find is basically -[NSString rangeOfString:…] but that method doesn't handle starts-with-word and ends-with-word so I added them in a category in OCR_NSString. Since that category is doing the actual work, I added a unit test.

Since Apple's Vision framework uses a completion callback, I can't just write:

for(page in pages){
  selection = Find(OCRText(page)); 
  if (selection){ NavigateTo(page); Show(selection); break; } 
}

Instead, I have to turn the for loop inside out - that's what OCRRangeEnumerator is for:

func completion(matchingText){
  if(matchingText){
    NavigateTo(page); Show(matchingText);
  } else {
     page = enumerator.Next()
     if (page) {
      OCRText(page, completion);
     } else {
        NotFound();
     }
  }
}
OCRText(enumerator.Next(), completion);

But, I have to schedule the call to OCRText with dispatch_async to keep from overflowing the stack, so I need to put up a modal progress dialog to that the user can't change the Find operation while it is running. As long as I've got a progress dialog, it has a cancel button so the user can cancel a long running Find.

Since all of this depends on 'Recognize Text' being turned on in Preferences, turning 'Recognize Text' off modifies the Edit menu to remove the inappropriate menu items. (Turning 'Recognize Text' back on restores those items.)

@nickv2002
Copy link
Collaborator

@DavidPhillipOster Thanks for all your work on this exciting feature! In another thread you talked about making a beta version of the app for people to try. Would it make sense to do that for this update?

@DavidPhillipOster
Copy link
Author

Done. Thanks for the nudge. https://github.com/DavidPhillipOster/Simple-Comic/releases/tag/1.9.4β - It's my first time doing a release on github, so please get in touch with me if there are problems.

@nickv2002
Copy link
Collaborator

I checked some files and did some searches with the beta version you posted. Works very well on my M1 MBA thanks!

Let's leave it up here for a week or so for other to test and then do a release to the App store version etc.

PS neat job on the icon. 😄

@DavidPhillipOster
Copy link
Author

I'm glad you liked it.

@nickv2002
Copy link
Collaborator

nickv2002 commented Jul 31, 2022

@DavidPhillipOster sorry I've been busy and I'm late to follow up here:

I tested the build you posted again recently and did some searches but couldn't get it to find any words that I typed in. I was trying many combinations including words that were clearly on the page and correctly able to be selected and copied. I was really confused and then I realize it was searching with case-sensitivity. Once I switched to Ignore Case it worked as expected again. I'm not sure if I had switched it off or if I had simply had the right case in my previous searches.

So since:

  1. Comic book speech bubbles etc are often written in all caps.
  2. MacOS text recognition often gets letter case wrong

It's probably better to just have it always be case-insensitive search. I think that would have minimal impact to the feature and avoid confusion as to why it didn't find what was expected.

If you can fix that, I'm happy to push out a new App Store build.

Thanks again for the awesome contribution!

@DavidPhillipOster
Copy link
Author

Commit f30a0e8 should do the trick for you. Thanks for the review!

@nickv2002 nickv2002 changed the base branch from arc to find August 1, 2022 18:37
@nickv2002 nickv2002 merged commit 96ce5e6 into MaddTheSane:find Aug 1, 2022
@nickv2002
Copy link
Collaborator

Merged to MaddTheSane:find. Will merge back to arc branch after some App Store tweaks...

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.

None yet

2 participants