Modify

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#12062 closed defect (fixed)

Doubleclick on checkbox labels doesn't work on filters added through the OR menu

Reported by: Ryan J Ollos Owned by: matobaa
Priority: normal Component: QueryUiAssistPlugin
Severity: normal Keywords:
Cc: Trac Release:

Description

Thank you for the nice plugin. I found it very useful and I'm working on adding the features to the Trac core in trac:#11417. Please feel free to comment on that ticket. Most likely the QueryUiAssistPlugin won't be needed by Trac 1.2. One fewer plugin is always nice :)

The issue I encountered is that the double-click doesn't function on the label of a checkbox when a filter is added using the OR selector.

I set off to fix this, and ended up making a few other changes as well:

  • Fixed implicitly declared variables
  • Optimized some jQuery selectors by limiting the scope of the search
  • Some PEP-0008 changes in the Python code.
  • Use prop rather than attr since Trac 1.0 uses jQuery 1.8.

The patch is attached. I hope it doesn't seem like I'm picking at your code. I learned far more from studying it than I'm contributing with the minor list of issues above :)

Attachments (3)

t12062.diff (3.7 KB) - added by Ryan J Ollos 5 years ago.
CustomQuery.png (31.9 KB) - added by Ryan J Ollos 5 years ago.
t12062-delegated-events.diff (2.2 KB) - added by Jun Omae 5 years ago.

Download all attachments as: .zip

Change History (18)

Changed 5 years ago by Ryan J Ollos

Attachment: t12062.diff added

Changed 5 years ago by Ryan J Ollos

Attachment: CustomQuery.png added

comment:1 Changed 5 years ago by matobaa

Status: newaccepted

Changed 5 years ago by Jun Omae

comment:2 Changed 5 years ago by Jun Omae

I think it would be simple and easy to use .on() method to handle delegated events rather than setTimeout(). See t12062-delegated-events.diff.

comment:3 Changed 5 years ago by matobaa

Resolution: fixed
Status: acceptedclosed

In 14387:

closes #12062

comment:4 Changed 5 years ago by Ryan J Ollos

There's a minor regression in [14387]. Double-click on a checkbox still selects the checkbox and clears all the neighbours, but double-click on the label of a checkbox no longer clear all the neighbours.

  • queryuiassistplugin/1.0/uiassist/htdocs/js/enabler.js

    diff --git a/queryuiassistplugin/1.0/uiassist/htdocs/js/enabler.js b/queryuiassi
    index f0dc07e..8ecd52d 100644
    a b  
    2121  function binder() {
    2222    var $filters = $("#filters");
    2323    $filters.on('dblclick', 'label', flip);
    24     $filters.on('dblclick', ':checkbox', selectone);
     24    $filters.on('dblclick', ':checkbox, :checkbox + label', selectone);
    2525  }
    2626
    2727  $(document).ready(function() { binder() })

comment:5 Changed 5 years ago by matobaa

Resolution: fixed
Status: closedreopened

thanks! I'll update soon...

comment:6 Changed 5 years ago by matobaa

Resolution: fixed
Status: reopenedclosed

comment:7 Changed 5 years ago by Ryan J Ollos

One more change. Although it appears to be harmless, the flip function is executed when double-clicking on a checkbox or its label. The following fixes the issue:

  • queryuiassistplugin/1.0/uiassist/htdocs/js/enabler.js

    diff --git a/queryuiassistplugin/1.0/uiassist/htdocs/js/enabler.js b/queryuiassi
    index 8ecd52d..de5ce4c 100644
    a b  
    2020  // bind "flip" above to labels in page.
    2121  function binder() {
    2222    var $filters = $("#filters");
    23     $filters.on('dblclick', 'label', flip);
    24     $filters.on('dblclick', ':checkbox, :checkbox + label', selectone);
     23    $filters.on('dblclick', 'th label', flip);
     24    $filters.on('dblclick', 'td :checkbox, td :checkbox + label', selectone);
    2525  }
    2626
    2727  $(document).ready(function() { binder() })

The addition of the th selector is what fixes the issue. The td selectors were just added for code clarity.

comment:8 Changed 5 years ago by matobaa

Resolution: fixed
Status: closedreopened

comment:9 Changed 5 years ago by Ryan J Ollos

New ticket for integrating this plugin to the Trac core is trac:#11970.

comment:10 Changed 5 years ago by Ryan J Ollos

I added a notice in QueryUiAssistPlugin@9. Please let us know if you have any suggestions about the changes in trac:#11970. My aim is to make this plugin no longer necessary by Trac 1.2, by having equivalent functionality in Trac.

comment:11 Changed 5 years ago by matobaa

Glad to hear that the function is now in Trac core, thank you!

comment:12 Changed 5 years ago by Ryan J Ollos

Is it okay to commit the change in comment:7?

comment:13 Changed 5 years ago by matobaa

yes!

comment:14 Changed 5 years ago by Ryan J Ollos

Resolution: fixed
Status: reopenedclosed

Thanks! Fixed in [14862].

comment:15 Changed 5 years ago by Ryan J Ollos

Changes in trac:#11970 have finally been committed (changeset:14225). Thanks Matobaa!

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain matobaa.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.