Keymapper documentation (and tests troubleshoot)

[rexagod]

In response to @sashadev-sky's suggestion here.

[sashadev-sky]

This is great!! Everything is really well-explained. @jywarren so I think we are starting with merging this one? Also noting status of my toolbar positioning PR does not matter here

The only changes I made are to use the Leaflet class factories syntax where available (I have been making a project out of updating the whole repo of these through FTOs so I am invested in it.

Also any ideas how we can avoid this testingIntent variable? Would you say that tests are not meant to create this kind of interdependency?. If I understand it correctly, it's allowing you to call _toggleRotateDistort without failing if the image editing class has not initialized yet? And doesn't have to do with your new code? If this is the case, you can initialize the editing in the beforeEach and then the below line shouldn't return undefined (see DistortableCollection test file for an ex.)

[rexagod]

If this is the case, you can initialize the editing in the beforeEach and then the below line shouldn't return undefined.

@sashadev-sky Can you detail a bit on what exactly do I define, since the undefined object here isn't used anywhere in the spec but the src file, and still gives undefined if I try to explicitly declare it in the beforeEach? Thanks!

[sashadev-sky]

Ohh ok I am going to clone your fork and try it out, I don't want to lead you down a path that doesn't work out

[sashadev-sky]

@rexagod I pulled your fork and you had a big problem ... you were were tracking the master branch of this repository and the main branch we work off of is actually called main. I pulled in main and rebased your branch on it, being careful to keep all your code the way it is in this PR.

I don't really understand how you saw, then, that my simulateCommandMousedown function should be moved, as this entire file (DistortableCollectionSpec) was not present in your repo until I rebased. Did you just see this in the browser or am I confused about something?

Anyway this rebase took me a full hour so I did not push the spec update I said I would. Will get back to that tomorrow. But now, if you look at DistortableCollectionSpec this is an example of what I was suggesting. I think if you define editing in beforeEach just like there, the other object will be in a test block

[rexagod]

@sashadev-sky So sorry for all this confusion, and also for taking up your time like this! I'm aware of the fact that much of PL's libs were shifted from master to main branch as the primary working branch. I would've left everything and got to fixing this had I known about the fact that I've pulled from the master before. It was very considerate of you to rebase when you found out about this, I can't thank you enough for all the conflicts that you would've have gone through, giving your time, while I can only guess how busy you are these days!

I'll get to fixing this, as this is my top priority now, and I'll see that everything's in place and working as you recommended above. Many apologies!

[rexagod]

@sashadev-sky I just checked out my keymapper docs commit and tried pull the master branch into it, but it showed a whole bunch of conflicts, which I think shouldn't have been the case if it was based of the master, also I ran a merge-base of the two branches and it came up with similar results, just to say, and I maybe very much wrong about this, but are you sure that the tracked branch was master and not main?

[sashadev-sky]

@rexagod If I rebased on main, it would make sense that it conflicts if you try to merge master in, right? Just wondering why do you want to pull the master branch in?

But I am pretty sure it was on master because I pulled your fork and the branch there was master. You didn't have some of the recent files that are on main. Then I pulled your remote branch in, and the upstream main branch, and I rebased keymapper-docs over main.

This is what my terminal output looks like:

You can see here that the only difference between public lab's main branch and keymapper-docs is the updates from this PR. Versus with master its now diverged. Some recent PR's are in the PL master branch but not most of the recent ones. (master is 49 commits behind)

Sorry if I changed anything that caused confusion!! I will fix it just not understanding the problem

[sashadev-sky]

@rexagod I looked again and 2 things:

1. running the below should solve your travis issue:

$ rm -rf package-lock.json node_modules
$ npm install

2. the intent variable doesn't make the test pass anymore after rebase. But without the code on this branch it passes so I think we should fix the code logic to pass the test instead of changing the test and introducing this variable

[rexagod]

beforeEach still isn't working, can you have a look @sashadev-sky? I've never been this stuck in a PR! 😕

Nonetheless, I'm trying to fix this.

[sashadev-sky]

@rexagod I had to do a big rebase again to remove the gitignore and package-lock.json. Your keymapper still works and everything should be the same for you, except I think I may have resolved a conflict with the README to actually keep your old README changes, because a lot of them were gone and I wanted to look through them to see which part we could leave for documentation here.

I recommend before you start working to do a git pull of this branch

[sashadev-sky]

@rexagod UI is perfect for both index and select view:

some quick things:

• Why RR? T is also a toggle activity but we just left that one as T
• I think you need to standardize all of these: like "toggle scale", "toggle rotate"
• I'll standardize them really quick right now and push it here! Let me know what you think!

[sashadev-sky]

@rexagod Ok good job! I standardized even further on top of your updates: do you like this UI change as well: (I'll revert those easily if you dont dw). Note I will probably make one hotkey to toggle send up / down instead of having two in my PR on normalizing hotkeys: #229. But for now doesn't matter

[sashadev-sky]

@rexagod ok so functionality is great! The next step is the placement of the code doesn't make sense in the initialize function for DistortableImage.Edit.js. Do you think it would be best if we moved the keymapper into its own file, KeymapperControl.js and make it a class that extends from control?

[sashadev-sky]

@rexagod get on gitter!

[sashadev-sky]

Ok so from our discussion: this is basically ready we just need to add an option for the user to pass keymapper: false during initialization. By default it will be there. Then I will ensure documentation is in order.

The next step will be to open a new PR and make it so that the keymapper is not hardcoded, but dynamic based on each tool added to the toolbar

[sashadev-sky]

@rexagod Ok I added the documentation let me know what you think. Obviously you can always revert my commit but I also saved it for you here just in case so you can see it easily already in md format: https://github.com/sashadev-sky/PL-notes/blob/master/save-notes.md

[sashadev-sky]

@rexagod Ok this is great!! Some things that could be improved, whether here or in a new PR:

1. Is it better if we make the keymapper and keymapper_position options all under one option by just making the possible values keymapper: topright (default) bottomleft, bottomright, bottomleft, hide (instead of false, for consistency, but would mean suppress it)? I actually don't know the answer to this myself.

2. ALTERNATIVE: Should we expose the keymapper initialization code in the example files to allow the user to pass position if they would like there? and keymapper: false would still be the ultimate overriding factor and this would be passed to the image?

3. Either way, we do need to break this connection of keymapper and distortableImageOverlay eventually because it doesn't make complete sense. Eventually when the featureGroup is for single and multiple images, we can make these options passed there instead of to individual images? And we move the keymapper initialization code in the featureGroup class, or don't have it in either by just using an "add hook", like after something else is done initializing, create the keymapper.

   • This will also fix problems like: For select.html, if the user wants to turn off the keymapper for multiple image select interface, they would have to manually pass keymapper: false to each image.

@jywarren what are your thoughts? But issues like point 3 are why I would recommend introducing a breaking version that requires the featureGroup sooner than later.

Note this is merge-able - it works well! But we do need to address how important these points on the implementation are right now as opposed to opening a PR to fix it later.

[rexagod]

Okay so the docs look good for now. Also, regarding the above review points, @sashadev-sky, would it be okay if I pushed those in another PR, since I think we can work with what's in this PR for now while continuing on the next one, and maybe we can take this discussion over there?

[jywarren]

This will also fix problems like: For select.html, if the user wants to turn off the keymapper for multiple image select interface, they would have to manually pass keymapper: false to each image.

I think this is the most important point -- for this reason it makes sense for the keymapper code to be outside the distortableImage code -- a kind of "add-on" interface you initialize in index.html.

It's a really great UI to have. But it's more a feature of a map, than of an image. Perhaps we should merge this now, then in a follow-up, let's break it out so that we don't have to set it on a per-image basis, but rather for the whole map. Could it live in a submodules like this:

Leaflet.DistortableImage/src/edit/DistortableImage.Edit.js

Line 3 in 6f55369

L.DistortableImage.Edit = L.Handler.extend({

So, like, L.DistortableImage.Keymapper? This would be really great and make good architectural sense too.

[sashadev-sky]

@rexagod @jywarren yes perfect.

Ok I just want to add to documentation about positioning. then I will do another lookover and merge.

@rexagod maybe let's not have it in the tools folder, that will be for toolbar tools? In the meantime should we move it out to just the edit folder?

@jywarren I think it is time to make breaking change with featureGroup after this. There will be so many things to rewrite at this point and it is going to save a lot of time for everyone. Making a feature work both in index.html and select.html is really time consuming I think. Both in careful thought about the best way to do it and then the actual implementation and avoiding creating bugs. Then much of this code will have to be tracked down afterwards and removed.

[sashadev-sky]

@rexagod @jywarren tomorrow I have personal matters to attend to all day so there might be a small delay in merge sorry

[sashadev-sky]

@rexagod @jywarren nevermind I felt bad dragging out. Opening a follow up issue for @rexagod and moving all relevant info there. good job here, @rexagod :)