41652 - Advanced Query: Use JavaScript to modify/create boolean charts

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P1)

Description

[Matthew Tuck [:CodeMachine]]

I'm well aware Terry's policy was not to require Javascript for users of
Bugzilla - and the advanced querying facility fits in with this.

But it would be nice if Javascript was used to alter the page instead, where
available.  This would be much quicker that loading the page again.

Not sure what DOM level this would require (1?), but I'm pretty sure it's
possible.  I guess implementation would involve a script that altered the button
behaviour where the DOM facilities are present.

Comment 1

[Stephan Niemz]

moving to real milestones...

Comment 2

[Matthew Tuck [:CodeMachine]]

Moving to new Bugzilla product ...

Comment 3

[Marc Schumann [:Wurblzap]]

Unless I misunderstand what this bug is about, this WORKSFORME by now.

Comment 4

[Rémi Zara]

Attachment: call new javascript

First part of patch: link new javascript page into the boolean_chart section of the search page, and modify event handlers.

Comment 5

[Rémi Zara]

Attachment: javascript to modify boolean charts client side

This javascript file contains code to alter the boolean charts client side.

Comment 6

[Max Kanat-Alexander]

Okay, now that we have an actual patch, let's make this bug's summary more specific.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 214220 [details] [diff] [review]
call new javascript

Looks good, r=myk

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 214221 [details]
javascript to modify boolean charts client side

>function alterBooleanChart(evt) {
>    evt = evt ? evt: window.event;

Nit: put a space before the colon, i.e.: evt: -> evt :


>    if (!document.getElementById) {
>        // not so much dom support
>        document.forms[0].action='query.cgi#chart';
>        document.forms[0].method='POST';
>        return 1;
>    }

Nit: I know the original code returned "1", but I think its intent is to return boolean "true", and since JavaScript (unlike Perl) actually has boolean primitives, and since you're rewriting the code anyway, use "true" here instead of "1".


>    // get the ids of the new chart elem to create
>    var addCmdIds = /cmd\-add(\d+)-(\d+)-(\d+)/.exec(evt.target.name).slice(1);
>    if (addCmdIds[2] != "0") {
>        // add a chart column (Or)
>        addChartCol(evt,addCmdIds);
>    } else if (addCmdIds[1] != "0") {
>        // add a chart row (And)
>        addChartRow(evt,addCmdIds);
>    } else {
>        // add a new chart
>        addChart(evt,addCmdIds);
>    }
>    // switch to POST method
>    document.getElementsByName("queryform")[0].method = "POST";
>    // prevent default action
>    evt.preventDefault();
>    evt.stopPropagation();
>    return false;
>}

Nit: capitalize "Id", i.e. addCmdIds -> addCmdIDs.
Nit: it would make this code more readable to add blank lines between logical blocks and the last return statement, i.e.:

    // get the ids of the new chart elem to create
    var addCmdIds = /cmd\-add(\d+)-(\d+)-(\d+)/.exec(evt.target.name).slice(1);
    if (addCmdIds[2] != "0") {
        // add a chart column (Or)
        addChartCol(evt,addCmdIds);
    } else if (addCmdIds[1] != "0") {
        // add a chart row (And)
        addChartRow(evt,addCmdIds);
    } else {
        // add a new chart
        addChart(evt,addCmdIds);
    }

    // switch to POST method
    document.getElementsByName("queryform")[0].method = "POST";

    // prevent default action
    evt.preventDefault();
    evt.stopPropagation();

    return false;
}


>/*
> * Add a new chart column (Or)
> * @param evt event on the add button
> * @param addCmdIds ids of the new chart elem to create
> */
>function  addChartCol(evt,addCmdIds) {

Nit: put spaces after commas, i.e.: evt, addCmdIds.


>    var negateInput = document.getElementById("negate"+addCmdIds[0])

Nit: put spaces around plus signs, i.e. "negate" + addCmdIds[0]


>    if (negateInput) {
>        // already exists
>        return;
>    }

Nit: omit brackets around one-line conditional and looping blocks, i.e.:

if (negateInput)
    return


Otherwise this code looks good, but while testing I noticed a regression in functionality: it's no longer possible to remove elements from boolean charts.  Without JavaScript, you can do so by going "Back" in your browser's history, but with JavaScript enabled you can no longer do that.  You should add a button to each row that removes the row from its chart so that we don't regress the functionality of boolean charts.

Comment 9

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 10

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?". Then, either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

This particular bug has not been touched in over eight months, and thus is being retargeted to "---" instead of "Bugzilla 4.0". If you believe this is a mistake, feel free to retarget it to Bugzilla 4.0.

Comment 11

[Frédéric Buclin]

pyrzak, I'm sure that's something you want to do. ;) This would avoid useless calls to the server when all we want is a new set of select fields.

Comment 12

[Guy Pyrzak]

Yeah this looks like a good one. I'll make sure to add it to the 4.0 bug list.

Comment 13

[Dave Miller [:justdave]]

A way to remove a row would be good while we're at it. (right now once you've added a row there's now way to get rid of it).

Comment 14

[Max Kanat-Alexander]

Seriously, somebody should finish this. :-)

Comment 15

[Guy Pyrzak]

We might be doing something like this at NASA and i should be able to port it over once I've got a design from our NASA designers. I'm sure JS for the boolean charts will be part of it.

Comment 16

[Guy Pyrzak]

it appears that currently boolean charts are not supported without JS. Therefore the new implementation in JS will not attempt to maintain any functionality beyond 1 boolean chart without JS being turned on. Please post if this is incorrect. Tested in FF3.5 on a mac

Comment 17

[Guy Pyrzak]

Attachment: Javascript Boolean Charts

allows for adding, removing and caching of dynamically added boolean charts.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 408332 [details] [diff] [review]
Javascript Boolean Charts

This is really cool! :-)

I haven't looked at the code yet, but here's a few comments:

1) The remove buttons don't seem to work quite right--Remove Chart removed my And, Remove And did nothing, and Remove Or submitted my form, for some reason.

2) I'd really like to see "Remove And" appear as an [x] link next to "And", "Remove Or" appear as an [x] link next to the chart row, and "Remove Chart" appear as an [x] next to the <hr> that creates the chart (or somewhere else reasonable).

That serializer thing looks pretty clever. :-)

Wow, I'm surprised that this patch was so big, but I'm so happy that it's done even though it WAS so large! :-)

Comment 19

[Max Kanat-Alexander]

Here's the bug with the Remove buttons:

1) Click Or
2) Click And on the last row
3) Click Add Chart on the last row
4) Try to Remove Chart (doesn't happen)

Now there's some confusion and the other Remove buttons don't work.

Comment 20

[Guy Pyrzak]

Attachment: V2

some bug fixes and replaced buttons for hyperlinks with [x]

Comment 21

[Guy Pyrzak]

Attachment: V2.1

I realized i needed to reserialize after i fix the chart numbers

Comment 22

[Guy Pyrzak]

Attachment: V3

Made some usability improvements. The page only refreshes now if the user has actually entered a boolean chart beyond the 1 that exists on the page normally. Also the serializer removes blank query parameters in an attempt to shorten the URL.

Comment 23

[Max Kanat-Alexander]

Comment on attachment 408409 [details] [diff] [review]
V3

Okay, I think I wasn't totally clear about my requests on where the [x] should go:

When I add a new Or chart, the [x] should go at the start or end of the chart. The first Or chart or any And set or any Chart should not have an [x]. (I can pretty easily clear those values if I want to, myself.)

When I add a new And chart, the word "and" is interposed between charts. The [x] should be next to the And, because that's the clearest point for the user to realize what it will be doing. (When you have an and and then two ors after it, it's very unclear which And will be deleted by the [x] after the "And" *button*.)

When you add another boolean chart, the [x] should go next to the <hr> that appears at the top of the chart. (Otherwise there's the same confusion about the [x] after "Add Another Boolean Chart", which appears on every item.)


Also, I think search-advanced is not the only template where boolean-charts.html.tmpl is used, so you may have to add your js stuff to other places. (And be wary of going over 80 characters for lines of code.)

Comment 24

[Guy Pyrzak]

Your request implies that a user can remove any boolean chart (or, and etc) from any location. Currently the user can only remove any boolean chart, not and/ors. Doing this would not be difficult but would require more code for this patch. I'd have to "fix" the boolean charts whenever someone removes one of them. Let me know if you want this as part of this patch or as a separate one. The ability to remove any arbitrary AND/OR is a new feature and I was just trying to not have a regression from the current boolean charts which only allow you to remove the last boolean chart.

Good point about more than just advanced search i'll do that.

Comment 25

[Frédéric Buclin]

For now, we should focus on what this bug is about: use JS to create boolean charts. Ability to delete them should go elsewhere.

Comment 26

[Frédéric Buclin]

pyrzak, any chance to attach an updated patch before we freeze?

Comment 27

[Max Kanat-Alexander]

Yeah, to be clear about this (pyrzak and I discussed this on IRC, and I didn't see this comment until recently because I wasn't CC'ed on this bug), I don't want any new functionality there as far as deleting, I just wanted the deletion links/buttons in a different place. :-)

Comment 28

[Frédéric Buclin]

Too late for 3.6.

Comment 29

[Nathan Tuggy (:tuggyne)]

Do we have another bug for removing rows filed yet? I didn't find anything....

Comment 30

[Max Kanat-Alexander]

  pyrzak: I'm going to *seriously* refactor Search.pm for 4.2, and boolean charts might end up working totally differently, so you might want to put this on the back-burner for a little while.

Comment 31

[Frédéric Buclin]

(In reply to comment #30)
>   pyrzak: I'm going to *seriously* refactor Search.pm for 4.2, and boolean
> charts might end up working totally differently, so you might want to put this
> on the back-burner for a little while.

Are you done with it now? Can pyrzak update his patch?

Comment 32

[Max Kanat-Alexander]

I'm not done with it, no. I'm in touch with pyzak about it, so I will for sure let him know when it's a good idea to start again.

Comment 33

[Frédéric Buclin]

(In reply to comment #32)
> I'm not done with it, no. I'm in touch with pyzak about it, so I will for sure
> let him know when it's a good idea to start again.

Ok, but it's also a good idea to let the community know about this. Your private talks are... private, and we have no idea what's going on here. ;)

Comment 34

[Max Kanat-Alexander]

Okay. Well, when this bug is ready to be worked on, either myself or pyrzak will comment in it.