Image drag and drop #670

divyabaid16 · 2019-06-06T06:19:28Z

Fixes #644 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

PR is descriptively titled 📑 and links the original issue above 🔗
tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
code is in uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updation
ask @publiclab/reviewers for help, in a comment below

GIF For the same:

Thanks!

codeclimate · 2019-06-06T06:20:11Z

Code Climate has analyzed commit f592c355 and detected 0 issues on this pull request.

View more on Code Climate.

jywarren · 2019-06-06T10:17:54Z

This looks great! @sashadev-sky what do you think?

…

On Wed, Jun 5, 2019, 11:20 PM codeclimate[bot] ***@***.***> wrote: Code Climate has analyzed commit f592c35 <f592c35> and detected *0 issues* on this pull request. View more on Code Climate <https://codeclimate.com/github/publiclab/mapknitter/pull/670>. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#670?email_source=notifications&email_token=AAAF6J4GPHOPRW5GEOMSRHLPZCUBXA5CNFSM4HU3LDG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXB3EOY#issuecomment-499364411>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6JYNWFLONRPILPVCKFLPZCUBXANCNFSM4HU3LDGQ> .

grvsachdeva · 2019-06-06T23:23:43Z

https://travis-ci.org/publiclab/mapknitter/jobs/542107150
Functional tests are failing

codecov · 2019-06-07T05:34:25Z

Codecov Report

Merging #670 into main will decrease coverage by 1.86%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##             main     #670      +/-   ##
==========================================
- Coverage   41.36%   39.49%   -1.87%     
==========================================
  Files          34       34              
  Lines        1308     1314       +6     
==========================================
- Hits          541      519      -22     
- Misses        767      795      +28

Impacted Files	Coverage Δ
app/controllers/images_controller.rb	`17.33% <16.66%> (-0.06%)`	⬇️
app/controllers/application_controller.rb	`35.48% <0%> (-25.81%)`	⬇️
app/controllers/sessions_controller.rb	`16.45% <0%> (-18.99%)`	⬇️

divyabaid16 · 2019-06-07T07:38:30Z

@gauravano
This test is failing because of newly added column position in warpables.
But it is not failing when I tried to run functional test in my terminal

divyabaid16 · 2019-06-07T16:35:09Z

app/views/images/_index.html.erb

-  <tbody>
-    <% @map.warpables.each do |warpable| %>
+  <tbody data-url = "<%= sort_images_path %>">
+    <% ary = @map.warpables.sort { |a, b| a.position <=> b.position } %>


<% ary = @map.warpables.sort { |a, b| a.position <=> b.position } %>

@gauravano
This position is not being recognized even though it is added into the table.
After adding the column position from the rails terminal and migrating it did resolve the problem for me.

@divyabaid16 I think this was just a caching problem running rake db:drop db:setup fixed it for me. So thats not a prob

sashadev-sky

========

Update: I created this bug in PR #630 when I removed average = 0. I uploaded PR to fix it now #675. So sorry!! Perhaps we should add testing for this function to protect it? @jywarren @gauravano

This PR is good to go but it might be good to merge the bug fix first and I can repull this to confirm its gone.

========
This is working great, but there is a new bug that I think really needs to be fixed here -- basically if you upload an image onto a new map and refresh the page, unless you have moved the image from the sidebar onto the map AND interacted with the image (even click is fine), rails will throw an error. This is because the map controller show action calls average_cm_per_pixel in map.rb. There, we define a warpable right when the image is created. But a warpable is not a placed_warpable until you place the image on the map and click on it, which is when it gets assigned nodes

mapknitter/app/models/map.rb

Lines 194 to 209 in 03deef1

    
           def average_cm_per_pixel 
        
             if !warpables.empty? 
        
               scales = [] 
        
               count = 0 
        
               placed_warpables.each do |warpable| 
        
                 count += 1 
        
                 res = warpable.cm_per_pixel 
        
                 res = 1 if res.zero? # let's not ever try to go for infinite resolution 
        
                 scales << res unless res.nil? 
        
               end 
        
               total_sum = (scales.inject { |sum, n| sum + n }) if scales 
        
               return total_sum / count if total_sum 
        
             else 
        
               0 
        
             end 
        
           end

sashadev-sky · 2019-06-08T04:36:31Z

app/views/images/_index.html.erb

-  <tbody>
-    <% @map.warpables.each do |warpable| %>
+  <tbody data-url = "<%= sort_images_path %>">
+    <% ary = @map.warpables.sort { |a, b| a.position <=> b.position } %>


@divyabaid16 I think this was just a caching problem running rake db:drop db:setup fixed it for me. So thats not a prob

divyabaid16 · 2019-06-10T17:05:27Z

@jywarren can this be merged?

jywarren · 2019-06-12T22:53:30Z

Pretty soon! Sorry, i want to link this also to the question about re-ordering in #640. Perhaps we need a unified ordering system, but let's discuss the UI implications over there and come to a decision together?

sashadev-sky · 2019-06-13T01:36:11Z

@divyabaid16 @jywarren this also needs to be retested after the bug fix in #675 I really think that one should be merged asap!

jywarren · 2019-07-26T16:45:22Z

Can we coordinate here to identify how to:

send a new ordering to LDI from MK (so this code would trigger reordering of the displayed images in the map)
send a new ordering from LDI to MK (the opposite)

so we can sync? I think on the MK end we may need a method like reorder(images) or something?

grvsachdeva · 2019-07-26T16:51:45Z

Hey @jywarren, we want the mentioned functionality for Action cable syncing feature right?

jywarren · 2019-07-26T16:57:23Z

Yes, that sounds right! But perhaps we can synchronize with the Leaflet map first, then once Action Cable gets merged we can build on top of that. Thanks!

…

On Fri, Jul 26, 2019 at 12:52 PM Gaurav Sachdeva ***@***.***> wrote: Hey @jywarren <https://github.com/jywarren>, we want the mentioned functionality for Action cable syncing feature right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#670?email_source=notifications&email_token=AAAF6JYMCEMO2AFLJ3PFGL3QBMTSDA5CNFSM4HU3LDG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD25EQ3A#issuecomment-515524716>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J5WRF7WHP77LOXDN6TQBMTSDANCNFSM4HU3LDGQ> .

grvsachdeva · 2019-07-26T17:16:07Z

can you clarify a bit, why we want to sync with LDI in-between? For syncing, we can just broadcast the coordinates of an image on a map in a channel and other users' view will be updated accordingly. Am I missing something?

Also, if there's any other strategy in mind, can you upload a rough sketch showing the whole data and control flow? Thanks!

jywarren · 2019-07-26T17:46:52Z

I guess there are 3 parts here: ordering in the sidebar, ordering the actual images, and then relaying either one to the other synchronous users. So, I'm fine with whichever order we want, i'm just trying to help structure the code flow. But i'm very open to ideas if someone wants to try planning this out and sharing a strategy? Thank you!

…

On Fri, Jul 26, 2019 at 1:16 PM Gaurav Sachdeva ***@***.***> wrote: can you clarify a bit, why we want to sync with LDI in-between? For syncing, we can just broadcast the coordinates of an image on a map in a channel and other users' view will be updated accordingly. Am I missing something? Also, if there's any other strategy in mind, can you upload a rough sketch showing the whole data and control flow? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#670?email_source=notifications&email_token=AAAF6J2NAPCROLT5S7RJGBLQBMWNPA5CNFSM4HU3LDG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD25GKMQ#issuecomment-515532082>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J3POCYP442ZDOOKRY3QBMWNPANCNFSM4HU3LDGQ> .

grvsachdeva · 2019-07-26T17:59:57Z

ordering in the sidebar

Currently, we allow sorting of images. We need to make it async (just thinking of improvements there too).

Ok, getting back to issues. So, ordering in the sidebar is like if 2 users have the same map open in their browser window then if image order in the sidebar is changed by one, it should be reflected on others. Right? We need to save the state of the images too, in an array, maybe.

Ordering of images on map

For this, the first implementation approach which come to my mind is #670 (comment).

But, @jywarren I would like to understand this --> #670 (comment) approach. Can you please upload a sketch of control flow?

@ViditChitkara what's your current plan?

jywarren · 2019-07-26T18:10:13Z

My thought was that each system (Leaflet map, sidebar list) ought to have a method available to accept a new image ordering programatically. Then when a change is made to either (OR we receive a new ordering from another user) each of the 2 displays can be reordered using their .order() method. Does this make sense?

grvsachdeva · 2019-07-26T18:28:34Z

Hmm, so on receiving the data we have to refresh that particular section of screen ie., sidebar or map. The receiving functions would be channel sender and receiver but from there we need to call a function which would inject the data into the client side via JS.

jywarren · 2019-08-16T16:24:50Z

Yes - makes sense. Can we split this in 2 - setting up events to trigger synchronicity between the sidebar ordering and the map ordering first, and then think about cross-user synchronization later? If we do a good job setting up the event listeners and triggers for sync in a single browser window, adding the extra Action Cable cross-user synchronization shouldn't be too hard as a next step, and we don't have to be blocked here. What do you think, @divyabaid16 ?

jywarren · 2019-08-16T16:26:50Z

I believe DistortableImage just uses Leaflet's default bringToFront() system from the API, so it shouldn't be too hard to reorder the map images.

https://github.com/publiclab/Leaflet.DistortableImage/#single-image-interface-1

Then, we'd need to set up a listener on the map to trigger sidebar reordering whenever you reorder the map. cc @sashadev-sky just to keep folks synced!

divyabaid16 mentioned this pull request Jun 6, 2019

Drag and drop added for images in the sidebar #645

Closed

5 tasks

divyabaid16 force-pushed the sort branch from f592c35 to b805730 Compare June 7, 2019 05:34

divyabaid16 force-pushed the sort branch 4 times, most recently from 609d4df to ab5e4a9 Compare June 7, 2019 07:34

divyabaid16 force-pushed the sort branch 2 times, most recently from 06ede26 to 4fa80d2 Compare June 7, 2019 16:29

Image drag and drop

6f258d4

divyabaid16 force-pushed the sort branch from 4fa80d2 to 6f258d4 Compare June 7, 2019 16:32

divyabaid16 commented Jun 7, 2019

View reviewed changes

divyabaid16 requested review from jywarren, grvsachdeva and sashadev-sky June 7, 2019 18:35

sashadev-sky requested changes Jun 8, 2019

View reviewed changes

sashadev-sky mentioned this pull request Jun 8, 2019

Reverse map refresh bug #675

Merged

5 tasks

jywarren closed this in #675 Jun 13, 2019

divyabaid16 reopened this Jun 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image drag and drop #670

Image drag and drop #670

divyabaid16 commented Jun 6, 2019

codeclimate bot commented Jun 6, 2019

jywarren commented Jun 6, 2019 via email

grvsachdeva commented Jun 6, 2019

codecov bot commented Jun 7, 2019 •

edited

Loading

divyabaid16 commented Jun 7, 2019

divyabaid16 Jun 7, 2019

sashadev-sky Jun 8, 2019

sashadev-sky left a comment •

edited

Loading

sashadev-sky Jun 8, 2019

divyabaid16 commented Jun 10, 2019

jywarren commented Jun 12, 2019

sashadev-sky commented Jun 13, 2019

jywarren commented Jul 26, 2019

grvsachdeva commented Jul 26, 2019

jywarren commented Jul 26, 2019 via email

grvsachdeva commented Jul 26, 2019

jywarren commented Jul 26, 2019 via email

grvsachdeva commented Jul 26, 2019

jywarren commented Jul 26, 2019

grvsachdeva commented Jul 26, 2019

jywarren commented Aug 16, 2019

jywarren commented Aug 16, 2019

	def average_cm_per_pixel
	if !warpables.empty?
	scales = []
	count = 0
	placed_warpables.each do \|warpable\|
	count += 1
	res = warpable.cm_per_pixel
	res = 1 if res.zero? # let's not ever try to go for infinite resolution
	scales << res unless res.nil?
	end
	total_sum = (scales.inject { \|sum, n\| sum + n }) if scales
	return total_sum / count if total_sum
	else
	0
	end
	end

Image drag and drop #670

Are you sure you want to change the base?

Image drag and drop #670

Conversation

divyabaid16 commented Jun 6, 2019

codeclimate bot commented Jun 6, 2019

jywarren commented Jun 6, 2019 via email

grvsachdeva commented Jun 6, 2019

codecov bot commented Jun 7, 2019 • edited Loading

Codecov Report

divyabaid16 commented Jun 7, 2019

divyabaid16 Jun 7, 2019

Choose a reason for hiding this comment

sashadev-sky Jun 8, 2019

Choose a reason for hiding this comment

sashadev-sky left a comment • edited Loading

Choose a reason for hiding this comment

sashadev-sky Jun 8, 2019

Choose a reason for hiding this comment

divyabaid16 commented Jun 10, 2019

jywarren commented Jun 12, 2019

sashadev-sky commented Jun 13, 2019

jywarren commented Jul 26, 2019

grvsachdeva commented Jul 26, 2019

jywarren commented Jul 26, 2019 via email

grvsachdeva commented Jul 26, 2019

jywarren commented Jul 26, 2019 via email

grvsachdeva commented Jul 26, 2019

jywarren commented Jul 26, 2019

grvsachdeva commented Jul 26, 2019

jywarren commented Aug 16, 2019

jywarren commented Aug 16, 2019

codecov bot commented Jun 7, 2019 •

edited

Loading

sashadev-sky left a comment •

edited

Loading