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

Activating edge matching on the selected feature even when no feature in other track aligns with the selected feature is confusing. #9

Closed
yeban opened this issue Apr 20, 2014 · 7 comments
Milestone

Comments

@yeban
Copy link

yeban commented Apr 20, 2014

So I select a feature and its edges do not match with any other feature in all tracks displayed. Yet, left-edge-match and right-edge-match classes are applied to the selected feature. This is confusing because a user will be tempted to scroll down to look for corresponding edge-matched feature and find none.

This happens because FeatureEdgeManager iterates through all tracks including the one containing the selected feature and thus matches the selected feature with itself. A feature will always edge-match on to itself.

Following rough patch (git diff) is a fix. I can send a PR if you agree.

diff --git a/www/JBrowse/FeatureEdgeMatchManager.js b/www/JBrowse/FeatureEdgeMatchManager.js
index cdb483c..5ae1227 100644
--- a/www/JBrowse/FeatureEdgeMatchManager.js
+++ b/www/JBrowse/FeatureEdgeMatchManager.js
@@ -92,8 +92,8 @@ var FeatureEdgeMatchManager = declare(null, {
             var target_track = trackdiv.track;
             // only DraggableHTMLFeatures and descendants should have track.edge_matching_enabled
             if (target_track && target_track.store && target_track.edge_matching_enabled)  {
-                if (verbose_edges)  {
-                    console.log("edge matching for: " + target_track.name);
+                if (target_track === rec.track) {
+                    return;
                 }

                 var featureStore = target_track.store;
@@ -125,6 +125,9 @@ var FeatureEdgeMatchManager = declare(null, {

                                     var ssmin = ssfeat.get('start');
                                     var ssmax = ssfeat.get('end');
+
+                                    var ssubdiv = rec.track.getFeatDiv(ssfeat);
+                                    var $ssubdiv = $(ssubdiv);
                                     for (var k=0; k < target_subfeats.length; k++)  {
                                         var tsfeat = target_subfeats[k];
                                         var tstype = tsfeat.get('type');
@@ -142,9 +145,11 @@ var FeatureEdgeMatchManager = declare(null, {
                                                     var $tsubdiv = $(tsubdiv);
                                                     if (ssmin === tsmin)  {
                                                         $tsubdiv.addClass("left-edge-match");
+                                                        $ssubdiv.addClass("left-edge-match");
                                                     }
                                                     if (ssmax === tsmax)  {
                                                         $tsubdiv.addClass("right-edge-match");
+                                                        $ssubdiv.addClass("right-edge-match");
                                                     }
                                                 }
                                             }
@yeban
Copy link
Author

yeban commented Apr 20, 2014

I realised I don't have a working WA setup, so won't be able to send a (tested) PR. I am sorry. The patch above is tested on https://github.com/yeban/afra.

yeban added a commit to wurmlab/afra that referenced this issue Apr 20, 2014
@S2KFan
Copy link
Contributor

S2KFan commented Apr 21, 2014

I don't see how this is "confusing because a user will be tempted to scroll down to look for corresponding edge-matched feature and find none". The selected feature is highlighted and it overrides the edge-matching so there shouldn't be any confusion there. When selecting a whole feature (double clicking), having the subfeatures selected helps enforce the fact that everything is selected.

@yeban
Copy link
Author

yeban commented Apr 21, 2014

.selected-annotation doesn't override .left-edge-match and .right-edge-match. They both add up. And it's visible in width of the border / outline around the feature. The border around a selected feature only should be 3px wide. The boundary around a selected feature that also edge-matches with other features should be 3px + 2px = 5px wide. CSS classes for left and right edge match seem to be defined twice by WA, on the demo server at least, so you will have to disable it in two places to see the effect. Now, If you are arguing that 2px would go unnoticed by a casual user, maybe. I noticed when I resized an exon and realised WA is still indicating an edge match ... scrolled down and found no edge-matched feature.

When selecting a whole feature, having the subfeatures selected is redundant, imo. When I see a border enclosing 3 rectangles (exons), I intuitively conclude all 3 are selected and not an imaginary outer container.

@yeban
Copy link
Author

yeban commented Apr 21, 2014

I hope the screenshots below make my point clear.

  • Selected feature with an edge-matched feature below.
    screen shot 2014-04-22 at 2 44 05 am
  • Selected feature resized. Right edge no longer matches with the feature below, but .right-edge-match is applied.
    screen shot 2014-04-22 at 2 44 58 am
  • Selected feature resized. Right edge no longer matches with the feature below, and .right-edge-match not applied.
    screen shot 2014-04-22 at 2 45 37 am

Thanks for your time.

@nathandunn nathandunn modified the milestones: 3.0 Release - Early 2015, Long-term Oct 2, 2014
@nathandunn nathandunn modified the milestones: 2.1, Long-term Apr 6, 2015
@nathandunn
Copy link
Contributor

Need to review this one again.

@nathandunn nathandunn modified the milestones: 2.1.0, 2.1.1 Sep 10, 2015
@nathandunn nathandunn modified the milestones: 2.1.1, 2.1.2 Mar 3, 2017
@nathandunn nathandunn removed this from the 2.1.5 milestone Dec 7, 2018
@nathandunn
Copy link
Contributor

diff --git a/client/apollo/js/FeatureEdgeMatchManager.js b/client/apollo/js/FeatureEdgeMatchManager.js
index 48fae5677..0e196c23f 100644
--- a/client/apollo/js/FeatureEdgeMatchManager.js
+++ b/client/apollo/js/FeatureEdgeMatchManager.js
@@ -110,6 +110,9 @@ var FeatureEdgeMatchManager = declare( null,
                 if (verbose_edges)  {
                     console.log("edge matching for: " + target_track.name);
                 }
+                if (target_track === rec.track) {
+                    return;
+                }
 
                 var featureStore = target_track.store;
 
@@ -145,6 +148,9 @@ var FeatureEdgeMatchManager = declare( null,
 
                                     var ssmin = ssfeat.get('start');
                                     var ssmax = ssfeat.get('end');
+
+                                    var ssubdiv = rec.track.getFeatDiv(ssfeat);
+                                    var $ssubdiv = $(ssubdiv);
                                     for (var k=0; k < target_subfeats.length; k++)  {
                                         var tsfeat = target_subfeats[k];
                                         var tstype = tsfeat.get('type');
@@ -159,12 +165,13 @@ var FeatureEdgeMatchManager = declare( null,
                                             if (tsid)   {
                                                 var tsubdiv = target_track.getFeatDiv(tsfeat);
                                                 if (tsubdiv)  {
-                                                    var $tsubdiv = $(tsubdiv);
                                                     if (ssmin === tsmin)  {
                                                         $(tsubdiv).addClass("left-edge-match");
+                                                        $(ssubdiv).addClass("left-edge-match");
                                                     }
                                                     if (ssmax === tsmax)  {
                                                         $(tsubdiv).addClass("right-edge-match");
+                                                        $(ssubdiv).addClass("right-edge-match");
                                                     }
                                                 }
                                             }

After implementing, the change is so slight, I don't think that most users could even tell, but I will leave the diff here for posterity.

@ghost ghost removed the discussion label Dec 7, 2018
nathandunn added a commit that referenced this issue Dec 7, 2018
@nathandunn nathandunn added this to the 2.2.0 milestone Dec 7, 2018
@nathandunn
Copy link
Contributor

Actually, I can see the reason for doing this. At some point a separate color for matches versus highglightin might be better, though.

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

No branches or pull requests

3 participants