GMOD / Apollo

Genome annotation editor with a Java Server backend and a Javascript client that runs in a web browser as a JBrowse plugin.
http://genomearchitect.readthedocs.io/
Other
127 stars 85 forks source link

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

Closed yeban closed 5 years ago

yeban commented 10 years ago

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 commented 10 years ago

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.

S2KFan commented 10 years ago

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 commented 10 years ago

.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 commented 10 years ago

I hope the screenshots below make my point clear.

Thanks for your time.

nathandunn commented 9 years ago

Need to review this one again.

nathandunn commented 5 years ago
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.

nathandunn commented 5 years ago

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