Don't clear the choice of blast method when the query sequence is deleted or changed, if appropriate #83

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@wwood
Contributor

wwood commented Mar 10, 2012

e.g. if the user inputs a nucleotide sequence,
chooses blastn,
then does a blast,
then deletes the whole sequence and puts in another nucleotide sequence,

the choice of 'blastn' should be left in place as it is likely
to be the method used. Currently all choices become available again.

At least from personal experience..

Signed-off-by: Ben J Woodcroft <gmail.com after donttrustben>

This is scratching my own itch, I guess, as I often seem to run a certain type of query consecutively. I realised actually that it is a bit of a temporary fix though - once we eliminate the blast methods section (#72) this will become redundant. Hence if you don't want to be bothered merging I totally understand.

Ben J Woodcroft
Don't clear the choice of blast method when the query sequence is del…
…eted or changed, if appropriate

e.g. if the user inputs a nucleotide sequence,
 chooses blastn,
 then does a blast,
 then deletes the whole sequence and puts in another nucleotide sequence,

the choice of 'blastn' should be left in place as it is likely
to be the method used. At least from personal experience..

Signed-off-by: Ben J Woodcroft <gmail.com after donttrustben>
@yeban

This comment has been minimized.

Show comment Hide comment
@yeban

yeban Mar 11, 2012

Don't clear the choice of blast method when the query sequence is deleted or changed, if appropriate

Umm, this was already resolved in 2d3b637 it seems.

yeban commented on ee4701d Mar 11, 2012

Don't clear the choice of blast method when the query sequence is deleted or changed, if appropriate

Umm, this was already resolved in 2d3b637 it seems.

This comment has been minimized.

Show comment Hide comment
@wwood

wwood Mar 11, 2012

Owner

Umm, this was already resolved in 2d3b637 it seems.

Well, given I didn't notice this bug until a week or so ago I doubted it was fixed 6 months ago. I rechecked by testing on the current official master branch (0874faa).

The problem as far as I can tell isn't that the choice doesn't get cleared when you remove the sequence, but does it gets cleared anyway when the next sequence gets pasted in. So keeping the choice upon clearing becomes effectively pointless.

Owner

wwood replied Mar 11, 2012

Umm, this was already resolved in 2d3b637 it seems.

Well, given I didn't notice this bug until a week or so ago I doubted it was fixed 6 months ago. I rechecked by testing on the current official master branch (0874faa).

The problem as far as I can tell isn't that the choice doesn't get cleared when you remove the sequence, but does it gets cleared anyway when the next sequence gets pasted in. So keeping the choice upon clearing becomes effectively pointless.

This comment has been minimized.

Show comment Hide comment
@yeban

yeban Apr 3, 2012

The problem as far as I can tell isn't that the choice doesn't get cleared when you remove the sequence, but does it gets cleared anyway when the next sequence gets pasted in. So keeping the choice upon clearing becomes effectively pointless.

I will blame jQuery for that: L122 and L126 don't work as expected. They uncheck valid blast method too and that shouldn't happen. So yeah, it is a valid issue that needs to be addressed. I think we should merge this even though #72 is more than half done (I am not sure when would #72 to make it to master). However, there is a simpler solution to the problem that I would prefer:

diff --git a/public/js/sequenceserver.js b/public/js/sequenceserver.js
index 970711a..09efbe7 100644
--- a/public/js/sequenceserver.js
+++ b/public/js/sequenceserver.js
@@ -16,7 +16,8 @@
 (function( $ ){
     //uncheck an element
     $.fn.uncheck = function() {
-        return this.removeAttr('checked');
+        this.attr('checked') && this.removeAttr('checked');
+        return this;
     };
 })( jQuery );

P.S: Sorry, this pull requests had completely slipped out of my mind.

yeban replied Apr 3, 2012

The problem as far as I can tell isn't that the choice doesn't get cleared when you remove the sequence, but does it gets cleared anyway when the next sequence gets pasted in. So keeping the choice upon clearing becomes effectively pointless.

I will blame jQuery for that: L122 and L126 don't work as expected. They uncheck valid blast method too and that shouldn't happen. So yeah, it is a valid issue that needs to be addressed. I think we should merge this even though #72 is more than half done (I am not sure when would #72 to make it to master). However, there is a simpler solution to the problem that I would prefer:

diff --git a/public/js/sequenceserver.js b/public/js/sequenceserver.js
index 970711a..09efbe7 100644
--- a/public/js/sequenceserver.js
+++ b/public/js/sequenceserver.js
@@ -16,7 +16,8 @@
 (function( $ ){
     //uncheck an element
     $.fn.uncheck = function() {
-        return this.removeAttr('checked');
+        this.attr('checked') && this.removeAttr('checked');
+        return this;
     };
 })( jQuery );

P.S: Sorry, this pull requests had completely slipped out of my mind.

This comment has been minimized.

Show comment Hide comment
@wwood

wwood Apr 3, 2012

Owner

sounds fine to me, and a lot simpler. Of course I'd rather #75 though..

Owner

wwood replied Apr 3, 2012

sounds fine to me, and a lot simpler. Of course I'd rather #75 though..

This comment has been minimized.

Show comment Hide comment
@yeban

yeban Apr 3, 2012

@wwood writes:

Of course I'd rather #75 though..

But we should fix this one in the mean time. Do you want to create a patch with my suggestions or you want to leave that to me?

yeban replied Apr 3, 2012

@wwood writes:

Of course I'd rather #75 though..

But we should fix this one in the mean time. Do you want to create a patch with my suggestions or you want to leave that to me?

This comment has been minimized.

Show comment Hide comment
@wwood

wwood Apr 3, 2012

Owner
Owner

wwood replied Apr 3, 2012

yeban added a commit that referenced this pull request Apr 3, 2012

ui: do not uncheck an appropriate blast method when sequence type cha…
…nges (#83)

I don't see any reason why the line:

    $("#blastp, #tblastn").uncheck().disable().first().change();

would uncheck all the blast methods instead of just `blastp` and `tblastn`,
because of which SS had the bug on the first place.  Embarrassingly, I don't
even understand why this patch actually fixes the problem :|.  Thanks to Ben,
for pointing out the issue and the solution :).

Signed-off-by: Anurag Priyam <anurag08priyam@gmail.com>
@yeban

This comment has been minimized.

Show comment Hide comment
@yeban

yeban Apr 3, 2012

Member

Fixed in 5be0eb5.

Member

yeban commented Apr 3, 2012

Fixed in 5be0eb5.

@yeban yeban closed this Apr 3, 2012

@yeban

This comment has been minimized.

Show comment Hide comment
@yeban

yeban Apr 7, 2012

Member

My solution was causing issues with database selection: wouldn't uncheck nucleotide if select protein later. And since #72 has moved to next, I removed the temporary fix (5be0eb5).

Member

yeban commented Apr 7, 2012

My solution was causing issues with database selection: wouldn't uncheck nucleotide if select protein later. And since #72 has moved to next, I removed the temporary fix (5be0eb5).

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