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

Fixed Maxbounds, Snap, and added Undo. #651

Closed
wants to merge 15 commits into from

Conversation

germanjoey
Copy link

Hello,

This is the L.Draw half of a compound PR I'm sending to L.Draw and
L.Snap. The changes to L.Snap require the changes to L.Draw, but the
changes to L.Draw does not require the changes to L.Snap.

Notes:

  • I don't have a touch-capable device available until at least
    Christmas, so if anyone can help check these changes w/ touch that would
    be great.
  • I folded in the changes to L.Snap in Leaflet-Draw/docs/examples/libs,
    so you can see all this stuff as
    Leaflet-Draw/docs/examples/crssimple_snapping.html
  • I had intended to add write some real documentation for all this, but
    this kiddo just won't sleep and I already had to merge all the changes
    after 0.4.3 by hand, so I figure I better send this in now before I get
    even more out of sync. I will send another PR with some actual words if
    there's interest in this PR.

Changelist:

L.Draw

  • Bugfix that forces L.Draw to respect a map's maxbounds
  • very important for using L.Draw with L.CRS.Simple
  • only does anything if map.options.maxbounds is not null
  • new drawn layers (for all shape types) must be inside the max bounds
  • edited layers (for both move and resize, for all shape types) are
    forced to stay inside the maxbounds
  • Some bugfixes to allow L.Snap to work in editmode with Leaflet 1.0
  • Various L.Draw events now pass much more information in the event
    object. (needed for undo/redo)
  • Undo/Redo implemented as L.Draw.UndoManager
  • works with all draw and edit shape types
  • bound to ctrl-z/ctrl-y by default, but customizable
  • Bugfixed Poly self-intersection logic for draw and edit

L.Snap

  • Various bugfixes to allow it to work with L.Draw for Leaflet 1.0
  • Various bugfixes in L.Snap logic
  • Snap now works for all shape types (i.e. polymove, rect, circle, and
    marker, not just polyline/polygon)
  • a new L.Draw.Guidelines class, which extends orthogonal lines to all
    sides of a bounding box during draw/edit for all rectangles and circles
    in drawnLayers
  • requires a bounding box, either a maxbounds on the map or passed in
    options
  • works with snap
  • a new L.Draw.Gridlines class, which draws a grid
  • requires a bounding box, either a maxbounds on the map or passed in
    options
  • can be set by either describing spacing or number of lines in the box,
    in either latlng or pixels
  • works with snap
  • snap will always prefer to snap to drawn features (or their vertices,
    if snapToVerticies is enabled) over Guidelines or Gridlines

Hello,

This is the L.Draw half of a compound PR I'm sending to L.Draw and
L.Snap. The changes to L.Snap require the changes to L.Draw, but the
changes to L.Draw does not require the changes to L.Snap.

Notes:
- I don't have a touch-capable device available until at least
Christmas, so if anyone can help check these changes w/ touch that would
be great.
- I folded in the changes to L.Snap in Leaflet-Draw/docs/examples/libs,
so you can see all this stuff as
Leaflet-Draw/docs/examples/crssimple_snapping.html
- I had intended to add write some real documentation for all this, but
this kiddo just won't sleep and I already had to merge all the changes
after 0.4.3 by hand, so I figure I better send this in now before I get
even more out of sync. I will send another PR with some actual words if
there's interest in this PR.

Changelist:

L.Draw
- Bugfix that forces L.Draw to respect a map's maxbounds
- very important for using L.Draw with L.CRS.Simple
- only does anything if map.options.maxbounds is not null
- new drawn layers (for all shape types) must be inside the max bounds
- edited layers (for both move and resize, for all shape types) are
forced to stay inside the maxbounds
- Some bugfixes to allow L.Snap to work in editmode with Leaflet 1.0
- Various L.Draw events now pass much more information in the event
object. (needed for undo/redo)
- Undo/Redo implemented as L.Draw.UndoManager
- works with all draw and edit shape types
- bound to ctrl-z/ctrl-y by default, but customizable
- Bugfixed Poly self-intersection logic for draw and edit

L.Snap
- Various bugfixes to allow it to work with L.Draw for Leaflet 1.0
- Various bugfixes in L.Snap logic
- Snap now works for all shape types (i.e. polymove, rect, circle, and
marker, not just polyline/polygon)
- a new L.Draw.Guidelines class, which extends orthogonal lines to all
sides of a bounding box during draw/edit for all rectangles and circles
in drawnLayers
- requires a bounding box, either a maxbounds on the map or passed in
options
- works with snap
- a new L.Draw.Gridlines class, which draws a grid
- requires a bounding box, either a maxbounds on the map or passed in
options
- can be set by either describing spacing or number of lines in the box,
in either latlng or pixels
- works with snap
- snap will always prefer to snap to drawn features (or their vertices,
if snapToVerticies is enabled) over Guidelines or Gridlines
@ddproxy
Copy link
Member

ddproxy commented Dec 14, 2016

I see breaking things.

You cool if I pull and piecemeal this?

@germanjoey
Copy link
Author

germanjoey commented Dec 14, 2016 via email

@ddproxy
Copy link
Member

ddproxy commented Dec 14, 2016

Sorry, not quite 'breaking' as much as there are conflicts. I'd like to separate the changes against the most recent build (which needs to be patched, desperately).

Also did you intend on adding the images to the commit?

@germanjoey
Copy link
Author

germanjoey commented Dec 14, 2016 via email

@germanjoey
Copy link
Author

germanjoey commented Dec 17, 2016 via email

@ddproxy
Copy link
Member

ddproxy commented Dec 18, 2016

@germanjoey Is this in a branch on your fork? I can pull directly from it.

@ddproxy
Copy link
Member

ddproxy commented Dec 18, 2016

Hey @germanjoey !

So, review https://rawgit.com/germanjoey/Leaflet.draw/master/docs/examples/full.html which should be running your fork. The branch breaks normal methods (outside of just snap).

Additionally you have committed .bak files by accident and the tests don't pass (npm run test or jake test). If you are using an IDE like intellij, you can wire the test suite to run the tests directly with Karma so you can get feedback immediately after making changes.

There are several bugs hidden within the PR, one is using map when it's not available in the function scope.

While I can't take the PR in full, I appreciate the work done here and would like to work with you to resolve these issues.

I'll check what I can do to get the Snap plugin either pulled into the Leaflet organization or better visibility into getting updates pushed that direction.

I've pushed a branch to https://github.com/Leaflet/Leaflet.draw/tree/snap-undo-bounds that has 0.4.7 rebased with your fork. There are a few fixes for the above issues but I hadn't completed them yet. I've also tweaked the package json and bower files to identify as 0.5.0 so I don't merge this on accident.

You can contact me directly at [email protected], https://gitter.im/Leaflet/Leaflet.draw occasionally and on skype at ddproxy .

Updated documentation.
@germanjoey
Copy link
Author

germanjoey commented Dec 19, 2016 via email

@germanjoey
Copy link
Author

germanjoey commented Dec 19, 2016 via email

@germanjoey
Copy link
Author

germanjoey commented Dec 19, 2016 via email

@germanjoey
Copy link
Author

germanjoey commented Dec 19, 2016 via email

@germanjoey
Copy link
Author

germanjoey commented Dec 19, 2016 via email

@ddproxy
Copy link
Member

ddproxy commented Dec 19, 2016 via email

Fixed some wacky stuff that would eventually happen if one did complex
transformations when adding and removing vertexes in edit mode, and then
undoing and redoing them repeatedly.
@germanjoey
Copy link
Author

germanjoey commented Dec 20, 2016 via email

Priority Queue for Undo's StateHandler to prevent key-triggered race
condition that could sometimes occur if the user smashed ctrlZ/ctrlY
very quickly.
Jon West and others added 2 commits January 30, 2017 23:12
@v1r0x
Copy link

v1r0x commented May 12, 2017

Any news on this? Do you need any help (e.g. testing)? Would like to help to get this merged 😉

@germanjoey
Copy link
Author

germanjoey commented May 25, 2017 via email

@v1r0x
Copy link

v1r0x commented May 26, 2017

Thanks for still working on it!
@ddproxy seems to be the only one who has worked on this project in the last couple of months. Maybe he can merge it (after testing of course). If he (or anyone else) is willing to merge it, I'd definitely test it!
I hope @ddproxy has some time or can @mention someone.

@v1r0x
Copy link

v1r0x commented Jun 27, 2017

@germanjoey Could you push your new changes? We decided to fork this repo and leaflet.snap as well, because we really need the features and your fixes. If this is ok for you, I try to help to fix conflicts and any errors. What do you think @germanjoey ?

@germanjoey
Copy link
Author

germanjoey commented Jun 29, 2017 via email

Updated Undomanager + some various bug fixes.
@ddproxy ddproxy self-requested a review July 5, 2017 14:24
@ddproxy
Copy link
Member

ddproxy commented Jul 5, 2017

@germanjoey Please check https://github.com/Leaflet/Leaflet.draw/tree/undo-manager for differences/compatibility, etc. This undo-manager has this PR and L.Draw 0.4.10 merged, code styles updated, etc. There's a bit of cleanup to be done in documentation and tests that should happen against that branch.

Fixed a number of various bugs from the merge, as well as got the
leaflet.draw-custom-src.js to go cleanly through jshint.
@germanjoey
Copy link
Author

germanjoey commented Jul 7, 2017 via email

@ddproxy
Copy link
Member

ddproxy commented Jul 7, 2017

Checkout the undo-manager branch and run jake for tests. There are a few failing. I'm also having problems drawing, anything, in the examples.

@germanjoey
Copy link
Author

germanjoey commented Jul 7, 2017 via email

@ddproxy
Copy link
Member

ddproxy commented Jul 7, 2017

I don't see it, so I'm not sure it actually happened... Git should have told you if it was unsuccessful. You can dump the git command and response here if you'd like.

@germanjoey check for invite to this repo...

Fix specs and examples
@germanjoey
Copy link
Author

germanjoey commented Jul 7, 2017 via email

@germanjoey
Copy link
Author

germanjoey commented Jul 10, 2017 via email

@ddproxy
Copy link
Member

ddproxy commented Jul 10, 2017

@germanjoey yes please, looking at this in the next 5 hours.

@germanjoey
Copy link
Author

germanjoey commented Jul 11, 2017 via email

@ddproxy
Copy link
Member

ddproxy commented Jul 20, 2017

@germanjoey I'm closing this PR in favor of working against the branch. Will have time to look at that a bit this evening and weekend.

@ddproxy ddproxy closed this Jul 20, 2017
@v1r0x
Copy link

v1r0x commented Sep 8, 2017

Sorry for commenting this old PR, but I wanted to ask status of the undo-manager branch? Didn't see any progress in a while. Is there something I could do to help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants