search

NaturalIntelligence / imglab

To speedup and simplify image labeling/ annotation process with multiple supported formats.
https://solothought.com/imglab/
MIT License
977 stars 596 forks source link

Add zoom, zoom in, & zoom out shortcuts #122

Open bonez0607 opened 5 years ago

bonez0607 commented 5 years ago

Purpose / Goal

Added zoom shortcuts specified in issue #107

Hello,

This is my first real PR - so I hope I'm doing things correctly...

I added a shortcut to 'click' the zoom button. From there the user can then use shortcuts to zoom in or out. I assumed this was the desired effect since the buttons aren't mounted until the zoom button is clicked, correct?

I'm on a mac and was having issues (even with existing shortcuts) with e.key == [appropriate key] && e.altKey working. However when I used the symbol created when pressing both alt and the event key it worked as expected.

For example e.key == '=' && e.altKey didn't work but e.key == '≠' did for the alt + '+' shortcut.

If I need to change to the previous formatting of e.key && e.altKey let me know and I can change back. I checked the shortcuts in Chrome, Firefox and Safari.

There were also a couple of quick mis commented items that I fixed.

Type

Please mention the type of PR

amitguptagwl commented 5 years ago

I added a shortcut to 'click' the zoom button. From there the user can then use shortcuts to zoom in or out. I assumed this was the desired effect since the buttons aren't mounted until the zoom button is clicked, correct?

Good point. I didn't think about that. We can probably move zoom in/out buttons to js functions. And the buttons in zoom-action.tag.html can just call those functions. In this way, a user needs not to open the zoom action bar to use the shortcuts.

For example e.key == '=' && e.altKey didn't work but e.key == '≠' did for the alt + '+' shortcut.

Need to be cross-checked but great if it works.

bonez0607 commented 5 years ago

That makes sense in regard to having them work without needing the action bar open. I can update it to reflect that.

Is there a specific js file you’d like the script to be in?

Lastly, would I need to open up a new PR or just amend this one?

amitguptagwl commented 5 years ago

Is there a specific js file you’d like the script to be in?

Probably common-actions.js

Lastly, would I need to open up a new PR or just amend this one?

this PR would be fine. I'll squash the changes before merging.

Important: Please check following once you complete the PR

  1. Import 2 images.
  2. Label 1st image. and zoom in/out
  3. again label it and zoom in/out
  4. check console if there is no error
  5. check if there is any change in labels position when zoom in/out

In case of confusion, you can share the screenshot or GIF for clear understanding.

bonez0607 commented 5 years ago

Alright I think I got everything working how it's supposed to. Just want to clarify some things.

  1. again label it and zoom in/out

Just want to clarify that the tags are supposed to scale with the image, correct?

No errors show in console

  1. check if there is any change in labels position when zoom in/out label scales with image and maintains position - See gif

imglab - image annotation tool 2

amitguptagwl commented 5 years ago

Yes! the labeled shapes are supposed to scale along with the image. But we've previously noticed that (due to a bug which is fixed now) if you label an image, change the zoom level, relabel and zoom again then the positions of label gets changed.

bonez0607 commented 5 years ago

Ok, the zoom functions have been added to a file called js/common-actions.js. And cleaned up from the other files.

amitguptagwl commented 5 years ago

So keyboard shortcuts along with direct button click are working fine?

amitguptagwl commented 5 years ago

Thanks. So keyboard shortcuts along with direct button click are working fine?

On Sun, 21 Oct 2018 at 20:02, Joe B. notifications@github.com wrote:

Ok, the zoom functions have been added to a file called common-actions.js. And cleaned up from the other files.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/NaturalIntelligence/imglab/pull/122#issuecomment-431673816, or mute the thread https://github.com/notifications/unsubscribe-auth/AHVgKNnnZDpR8xbi4WZVjaXTp0wLK_CFks5unIVogaJpZM4Xbki4 .

-- Cheers Amit Gupta

amitguptagwl commented 5 years ago

@bonez0607 Have you get the chance to test it from buttons as well as from shortcuts?

bonez0607 commented 5 years ago

Sorry, it's was a busy weekend. I can take a look to double check tomorrow night.

amitguptagwl commented 5 years ago

Sure no problem.

bonez0607 commented 5 years ago

So the zoom shortcuts work and the buttons work w/out an error. However, when using the keyboard shortcut the percentage increments/decrements by 20% rather than the 10% change when using the buttons. I checked to make sure nothing was being called twice, as that seemed like a potential culprit.

amitguptagwl commented 5 years ago

I hope you must have console log to check if that function is being called twice. and You must have prevented event propagation.

bonez0607 commented 5 years ago

Was able to track down the offending code. Checked in Chrome, Firefox, and Safari - buttons and keyboard are working.

amitguptagwl commented 5 years ago

great. Now you have to remove the console.log statement from your code. I'll suggest you review your code one more time, just to avoid any bug in future.

bonez0607 commented 5 years ago

Doh! ok - should be good.

amitguptagwl commented 5 years ago

Thanks for the changes. I'll separately check your changes after 2 days before merge.