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.

Sign up for GitHub

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

Jump to bottom

(WIP)Add ".tiff" as content upload type #577

Closed
wants to merge 1 commit into from
Closed

(WIP)Add ".tiff" as content upload type #577

wants to merge 1 commit into from

Conversation

cesswairimu
Copy link
Collaborator

Fixes #206

  • 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

codeclimate[bot]
codeclimate bot reviewed May 2, 2019
View reviewed changes
app/models/warpable.rb Outdated
@@ -11,7 +11,7 @@ class Warpable < ActiveRecord::Base
:small=> "240x180",
:thumb => "100x100>" }

validates_attachment_content_type :image, :content_type => ["image/jpg", "image/jpeg", "image/png", "image/gif"]
validates_attachment_content_type :image, :content_type => ["image/jpg", "image/jpeg", "image/png", "image/gif", "image/tiff"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [128/80]

app/models/warpable.rb Outdated
@@ -11,7 +11,7 @@ class Warpable < ActiveRecord::Base
:small=> "240x180",
:thumb => "100x100>" }

validates_attachment_content_type :image, :content_type => ["image/jpg", "image/jpeg", "image/png", "image/gif"]
validates_attachment_content_type :image, :content_type => ["image/jpg", "image/jpeg", "image/png", "image/gif", "image/tiff"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer single-quoted strings when you don't need string interpolation or special symbols.

app/models/warpable.rb Outdated
@@ -11,7 +11,7 @@ class Warpable < ActiveRecord::Base
:small=> "240x180",
:thumb => "100x100>" }

validates_attachment_content_type :image, :content_type => ["image/jpg", "image/jpeg", "image/png", "image/gif"]
validates_attachment_content_type :image, :content_type => ["image/jpg", "image/jpeg", "image/png", "image/gif", "image/tiff"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer single-quoted strings when you don't need string interpolation or special symbols.

@codeclimate
Copy link

codeclimate bot commented May 2, 2019

Code Climate has analyzed commit 9003a465 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Style 3

View more on Code Climate.

@codecov
Copy link

codecov bot commented May 2, 2019 •
edited
Loading

Codecov Report

Merging #577 into main will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #577   +/-   ##
=======================================
  Coverage   72.43%   72.43%           
=======================================
  Files          35       35           
  Lines        1335     1335           
=======================================
  Hits          967      967           
  Misses        368      368
Impacted Files Coverage Δ
app/models/warpable.rb 87.5% <100%> (ø) ⬆️

@cesswairimu cesswairimu force-pushed the accept-tiff-extensions-images branch 5 times, most recently from 789d2d0 to eabb55f Compare May 2, 2019 00:54
@jywarren
Copy link
Member

jywarren commented May 2, 2019 via email

@jywarren
Copy link
Member

jywarren commented May 2, 2019

Actually this should be all good; i'm wondering if a .tiff can be exported in mapknitter-exporter, and we can test that later!

@cesswairimu
Copy link
Collaborator Author

cesswairimu commented May 2, 2019 •
edited
Loading

Hey @jywarren, The upload is working fine but placing the image on the map fails. The js error returned is No compass data found I narrowed it down to this https://github.com/publiclab/mapknitter/blob/main/app/assets/javascripts/mapknitter/Map.js#L300-L310 but not sure why this is happening. Also the thumbnail is not visible.Any ideas?

@jywarren
Copy link
Member

jywarren commented May 2, 2019 via email

@cesswairimu
Copy link
Collaborator Author

Sure, I will comment if for now and work with @rexagod on removing the function. thanks @jywarren

@jywarren
Copy link
Member

jywarren commented May 2, 2019

Just feel free to ping me if this should be merged! Also, @cesswairimu I would be fine sharing merge rights on MapKnitter with you if you're comfortable with it. You can (should?) still feel free to ask for reviews from others before merging, but it may speed the process a bit. Thanks!

@rexagod
Copy link
Member

rexagod commented May 3, 2019 •
edited
Loading

Always excited to collab with Cess! 🚀 😄

@cesswairimu I can't push to this branch. Can you add me as a collaborator to your fork of this repo?

@jywarren I made the requested changes to the EXIF PR a week ago, and I do understand that you're very busy these days so can you please check it out whenever you have some time? Thanks!

@rexagod
Copy link
Member

rexagod commented May 3, 2019

Also, I added a new comment to the feature detection issue but no rush with this, and thank you @jywarren for mentioning me here!

@cesswairimu
Copy link
Collaborator Author

cesswairimu commented May 3, 2019 •
edited
Loading

Thanks @rexagod 😃 , I just added you as a collaborator

@jywarren
Copy link
Member

jywarren commented May 3, 2019 via email

@cesswairimu
Copy link
Collaborator Author

Just noting I tried commenting out the function part, but that did not fix the 'place' still looking into it though.

@rexagod
Copy link
Member

rexagod commented May 3, 2019 •
edited
Loading

Cess can you get on Gitter? I need to clarify a few things. @jywarren, no problem! 👍

@rexagod
Copy link
Member

rexagod commented May 3, 2019

Just noting I tried commenting out the function part, but that did not fix the 'place' still looking into it though.

Cess, just to confirm, did you do anything other than removing both fns and commenting out their calls (i.e., anything other than the diff below)?

@@ -237,105 +237,6 @@ 
-  geocodeImageFromId: function(dom_id,id,url) {
-    window.mapKnitter.geocodeImage(
-      $(dom_id)[0],
-      function(lat,lng,id,angle,altitude) {
-        /* Display button to place this image with GPS tags. */
-        $('.add-image-gps-'+id).attr('data-lat',lat);
-        $('.add-image-gps-'+id).attr('data-lng',lng);
-        if (angle) $('.add-image-gps-'+id).attr('data-angle',angle);
-        if (altitude) $('.add-image-gps-'+id).attr('data-altitude',altitude);
-        $('.add-image-gps-'+id).show();
-        $('.add-image-gps-'+id).on('click',function() {
-          $('.add-image-'+id).hide();
-          $('#uploadModal').modal('hide');
-          window.mapKnitter._map.setZoom(18);
-          window.mapKnitter._map.setView(
-            [$(this).attr('data-lat'),
-             $(this).attr('data-lng')]);
-          var angle = $(this).attr('data-angle') || 0;
-          var altitude = $(this).attr('data-altitude') || 0;
-          img = window.mapKnitter.addImage(url,
-                                     id,
-                                     $(this).attr('data-lat'),
-                                     $(this).attr('data-lng'),
-                                     angle,
-                                     altitude);
-          $('#warpable-'+id+' a').hide();
-        })
-      },
-      id
-    )
-  },
-
-  /*
-   * Accepts an image element, and executes given function with 
-   * params as: function(lat,lng) {}
-   * Adapting from: 
-    https://github.com/publiclab/mapknitter/blob/6e88c7725d3c013f402526289e806b8be4fcc23c/public/cartagen/cartagen.js#L9378
-  */
-  geocodeImage: function(img,fn,id) {
-    EXIF.getData(img, function() {
-      var GPS = EXIF.getAllTags(img)
- 
-      /* If the lat/lng is available. */
-      if (typeof GPS["GPSLatitude"] !== 'undefined' && typeof GPS["GPSLongitude"] !== 'undefined'){
-
-        // sadly, encoded in [degrees,minutes,seconds] 
-        var lat = (GPS["GPSLatitude"][0]) + 
-                  (GPS["GPSLatitude"][1]/60) + 
-                  (GPS["GPSLatitude"][2]/3600);
-        var lng = (GPS["GPSLongitude"][0]) + 
-                  (GPS["GPSLongitude"][1]/60) + 
-                  (GPS["GPSLongitude"][2]/3600);
-
-        if (GPS["GPSLatitudeRef"] != "N")  lat = lat*-1
-        if (GPS["GPSLongitudeRef"] == "W") lng = lng*-1
-      }
-
-      // Attempt to use GPS compass heading; will require 
-      // some trig to calc corner points, which you can find below:
-
-      var angle = 0; 
-      // "T" refers to "True north", so -90.
-      if (GPS["GPSImgDirectionRef"] == "T")
-        angle = (Math.PI / 180) * (GPS.GPSImgDirection["numerator"]/GPS.GPSImgDirection["denominator"] - 90);
-      // "M" refers to "Magnetic north"
-      else if (GPS["GPSImgDirectionRef"] == "M")
-        angle = (Math.PI / 180) * (GPS.GPSImgDirection["numerator"]/GPS.GPSImgDirection["denominator"] - 90);
-      else
-        console.log("No compass data found");
-
-      console.log("Orientation:",GPS["Orientation"]) 
-
-      /* If there is orientation data -- i.e. landscape/portrait etc */
-      if (GPS["Orientation"] == 6) { //CCW
-        angle += (Math.PI / 180) * -90
-      } else if (GPS["Orientation"] == 8) { //CW
-        angle += (Math.PI / 180) * 90
-      } else if (GPS["Orientation"] == 3) { //180
-        angle += (Math.PI / 180) * 180
-      }
- 
-      /* If there is altitude data */
-      if (typeof GPS["GPSAltitude"] !== 'undefined' && typeof GPS["GPSAltitudeRef"] !== 'undefined'){
-        // Attempt to use GPS altitude:
-        // (may eventually need to find EXIF field of view for correction)
-        if (typeof GPS.GPSAltitude !== 'undefined' && 
-            typeof GPS.GPSAltitudeRef !== 'undefined') {
-          altitude = (GPS.GPSAltitude["numerator"]/GPS.GPSAltitude["denominator"]+GPS.GPSAltitudeRef);
-        } else {
-          altitude = 0; // none
-        }
-      } 
-
-      /* only execute callback if lat (and by 
-       * implication lng) exists */
-      if (lat) fn(lat,lng,id,angle,altitude);
-    }); 
-  },
-
@@ -23,7 +23,7 @@
               <!-- <a href="">Re-place</a>  -->
               <script>
                 jQuery(document).ready(function($) {
-                  window.mapKnitter.geocodeImageFromId('#warpable-img-<%= warpable.id %>',<%= warpable.id %>,'<%= warpable.image.url(:medium) %>');
+                  # window.mapKnitter.geocodeImageFromId('#warpable-img-<%= warpable.id %>',<%= warpable.id %>,'<%= warpable.image.url(:medium) %>');
                 })
               </script>
             <% end %>
@@ -94,7 +94,7 @@
                       <script>
                         $('#warpable-{%= file.id %} [rel=tooltip]').tooltip();
                         $("#warpable-img-preview-"+{%= file.id %}).on('load', function() {
-                          window.mapKnitter.geocodeImageFromId("#warpable-img-preview-"+{%= file.id %},{%= file.id %},'{%= file.url %}'); 
+                          # window.mapKnitter.geocodeImageFromId("#warpable-img-preview-"+{%= file.id %},{%= file.id %},'{%= file.url %}'); 
                         })
                     {%# "</sc"+"ript>" %}
                   </td>

@cesswairimu
Copy link
Collaborator Author

cesswairimu commented May 3, 2019 •
edited
Loading

Nope, I just commented them out as ☝️ and removed them where called in the views

@cesswairimu cesswairimu added the on-hold Holding this until something that depends on it is fixed/implemented label Jun 17, 2019
@cesswairimu cesswairimu force-pushed the accept-tiff-extensions-images branch from eabb55f to 44dcd7a Compare July 9, 2019 21:25
@cesswairimu
Copy link
Collaborator Author

This PR has been open for a long time and no activity/ reviews requested has not been addressed. We understand you might be busy and engaged on other things. I am closing this for now...please feel free to reopen if you get some time and would like to finish this. Rem to check if its still available before you reopen. Thanks 😆 😆

@cesswairimu
Copy link
Collaborator Author

talk about saved replies to your own PR 🤣

@SidharthBansal
Copy link
Member

Well done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

Assignees
No one assigned
Labels
on-hold Holding this until something that depends on it is fixed/implemented
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accept geotiff (extension .tif)
4 participants
@cesswairimu @jywarren @rexagod @SidharthBansal