*** Bug 80168 has been marked as a duplicate of this bug. ***
I was not convinced by your description, but I tried the patch and was pleasantly surprised. It really makes sense, if the keyword list is small enough. However, I don't think this solution will scale to the number of keywords in use at mozilla.org . Keyword groups, or even hierarchical keyword trees, should be implemented either before or together with this, so that the system can have sub-menues. (Maybe even sub-sub-menues, but this could trigger other usability problems if the menu hierarchy gets too complex.) Without keyword groups, it is probably still faster to simply type the keyword you want to add, than to search for it in an overly long list. Minor nit: There could be better support for the "," separator used in the keyword textbox. Right now, the comma appears in the "Remove" menu, and the changes performed by the "Add" button don't do not generate commas, so that the field ends up as mixed style. Another small nit: You are able to add a keyword twice, and if you remove it, the keyword still occurs in the "Remove" menu. What about long keywords? Don't they make the "Remove" and "Add" buttons too wide?
The recently attached patch fixes the comma problem, prevents keywords from being added twice, and is more robust and documented.
The alert "That keyword is already in the keywords field!" is displayed only if I try to add it twice in a row. If I add a keyword that is already in the keywords field but which is not the first one, then the alert is not displayed. Instead, the original instance is deleted from the list, and it is inserted as the first keyword, so altogether it appears that it is moved to the beginning of the list. I'd prefer it if the dialog would always be thrown when you add a keyword that is already in the list. Sometimes removing a keyword doesn't work. It disappears from the "Remove" menu but the keywords field does not change. Currently, this problem occurs with all keywords but the first one. This problem and the previous one may have the same root cause.
The new attachment fixes the problems Andreas reported by working around a problem NS4.x had with the regular expression that finds keywords in the keyword field.
Does this patch depend on other patches that haven't been checked in yet? When I apply the latest patch to bug_form.pl and update to the CVS tip, I get the following software error when trying to view a bug: select bit, description, (bit & 128 != 0) from groups where bit & 1016 != 0 and isbuggroup != 0 and (isactive = 1 or (bit & 128 != 0)) order by bit: Unknown column 'isactive' in 'where clause' at globals.pl line 195.
Never mind. I ran checksetup.pl and it added that column for me. The only remaining issue with this patch is a minor one: When there are no keywords and you add more than one, then they are separated by a single whitespace (" "), but without a comma. The default separator is a comma followed by a whitspace (", "), and it would be nice if this separator could be used even in this case. Otherwise, it's great. :-)
Works fine now. t(ested)=afranke on Netscape 4.76 / Solaris. It would be nice to see this in the next release.
That's a whacking great chunk of JS to add to an already-bloated page. Is the added function really worth it? Gerv
If it was external JS wouldn't there be cache improvements? The file should rarely never change.
We would reduce the bulk significantly by changing the comments to template comments after templatisation. Gerv
We are currently trying to wrap up Bugzilla 2.16. We are now close enough to release time that anything that wasn't already ranked at P1 isn't going to make the cut. Thus this is being retargetted at 2.18. If you strongly disagree with this retargetting, please comment, however, be aware that we only have about 2 weeks left to review and test anything at this point, and we intend to devote this time to the remaining bugs that were designated as release blockers.
Comment on attachment 38530 [details] [diff] [review] uses commas as separators This needs-work because it's pre-templatization stuff. But the feature itself sounds good, some optional assistance with keywords is certainly necessary.
Created attachment 119350 [details] [diff] [review] Patch to add keywords to bug.choices variable Adds keyword array to bug.choices variable.
I've posted a couple of attachments (attachment ) as a proposed solution for this. Basically this change includes three things: 1) Add list of all available keywords to bug.choices variable exposed to template. 2) Add js file containing all functions 3) Replace the [% IF bug.use_keywords %] block surrounding the keywords text box with the code below. I couldn't get cvs to respond so I could create a patch file, here's the changes to template/en/default/edit.html.tmpl: Replace: <pre> [% IF bug.use_keywords %] <tr> <td align="right"> <b> <a href="describekeywords.cgi"><u>K</u>eywords</a>: </b> <td colspan="5"> <input name="keywords" accesskey="k" value="[% bug.keywords.join(', ') FILTER html %]" size="60"> </td> </tr> [% END %] </pre> with: <pre> [% IF bug.use_keywords %] <tr> <td align="right"> <b> <a href="describekeywords.cgi"><u>K</u>eywords</a>: </b> <td colspan="5"> <input type="hidden" name="keywords" value="[% bug.keywords.join(', ') FILTER html %]" /> <table width="100%" border="0" cellspacing="1" cellpadding="0"> <tr> <td width="25%"><label title="List of remaing keywords available to describe this issue"><font size="-1"><b>Available</b></font></label></td> <td width="12%"> </td> <td width="63%"><label title="List of keywords that apply to this issue"><font size="-1"><b>Selected</b></font></label></td> </tr> <tr> <td width="25%"> <select name="availableKeywords" multiple size="6"> [% FOREACH keyword = bug.choices.legal_keywords %] [% IF !bug.keywords.join(', ').match(keyword) %] <option value="[% keyword FILTER html %]">[% keyword FILTER html %]</option> [% END %] [% END %] </select> </td> <td width="12%"> <input type="button" name="Submit" value=">>" onclick="updateKeywords (this.form.name, 'availableKeywords', this.form.name, 'selectedKeywords', 'keywords', 'selectedKeywords')"/> <br/> <br/> <input type="button" name="Submit2" value="<<" onclick="updateKeywords (this.form.name, 'selectedKeywords', this.form.name, 'availableKeywords', 'keywords', 'selectedKeywords')"/> </td> <td width="63%"> <select name="selectedKeywords" MULTIPLE size="6" onchange="concatKeywords('changeform', 'keywords', 'selectedKeywords');"> [% FOREACH keyword = bug.choices.legal_keywords %] [% IF bug.keywords.join(', ').match(keyword) %] <option value="[% keyword FILTER html %]">[% keyword FILTER html %]</option> [% END %] [% END %] </select> </td> </tr> </table> </td> </tr> [% END %] </pre>
Created attachment 119891 [details] Patches to enhance keywords UI This is a zip of individual patch files as well as the js file all in one. The previous patches did not take into account keywords are used in creation of bugs...now it does.
Comment on attachment 119891 [details] Patches to enhance keywords UI Requesting review of patch
getting it out of my queue until the patches are reviewed
Jon: best to attach patches as cvs diff -uN - and, if you are attaching ZIPs, mark them as "application/zip", rather than patch (which comes down as text/plain.) Does your patch implement the function as specced in the initial report? If so, I'm not convinced it's something we actually want. The show_bug page is complex enough that any UI additions have to meet a fairly strong standard of usefulness. In this case, we aren't even adding a new field or new function, merely adding two new UI elements for controlling an existing field. I haven't seen any bug reports from Bugzilla users complaining that the keywords field is hard to manage. Has anyone else? Gerv
Comment on attachment 119891 [details] Patches to enhance keywords UI These patches fail to apply on a tip install in multiple places. Can you post an updated patch? As Gerv says, "cvs diff -uN <filenames>" is ideal, although "cvs diff -u <filenames>" plus the JS file as a separate attachment is ok if you don't have CVS write access.
i've found the added functionality provided by this patch to be very useful -- especially when dealing with large groups of keywords. i've gotten positive responses from the techs who use our install, and they prefer the patched version to the non-patched version. i have it currently working on a install derived from the cvs-tip, so i will look into creating a patch for you guys to possibly use/review/etc. more to come...
Created attachment 122700 [details] [diff] [review] diffs for Bug.pm, enter_bug.cgi, create.html.tmpl, edit.html.tmpl, header.html.tmpl there are the diffs with cvs-tip. tested it, seems to work. test it yourself to make sure. select.js must be added separately.
Created attachment 122701 [details] select.js used with attachment 122700 [details] [diff] [review] (above) put in js/ it differs slightly in size, so i'm attaching this one again.
Comment on attachment 122701 [details] select.js used with attachment 122700 [details] [diff] [review] (above) put in js/ Marking in accordance with review of companion patch.
Taking. Unawares of this bug, I implemented this very thing for a private installation of Bugzilla. I showed it to myk who seemed to like it, and it addresses all of myk's concerns in comment 31. I will try and backport it this week.
I tried this patch and it works fine as long as you are using netscape and/or IE. It does not work under Conquer.
Also Sorry to spam but this patch does not include a solution for editing multiple bugs at once. It only provides the new UI for new bugs and editing a single bug at a time.
Created attachment 139511 [details] [diff] [review] Patch to attachment select.js #122701 adds alpha sorting I have added a sort funciton that helps to keep the select boxes easy to read. I am using the select.js in production environment and it works really well. Feedback has been excelent.
Created attachment 151556 [details] The main keyword chooser file. - The is the mail js file which handles the widgets on the keyword chooser.
Created attachment 151557 [details] The keyword chooser template file This is the keyword chooser template used by the keyword chooser js file.
Created attachment 151558 [details] (deleted) Since the files are changed so much that patch does not make sense - attaching the entire create.html.tmpl file - so that the relevant code to keyword chooser can be applied.
Chris: the attachments just made on this bug (I guess there's one or two coming yet) are the code you did for AOL (referenced in comment 33) that they just released to us (copyright has been reassigned to the Mozilla Foundation according to the email I got on the subject relayed from AOL Legal). Since it's now legal to be touched, and you're the original author, it'd make perfect sense for you to be the one to finish backporting it, if you have time for it that is. :) Let us know :) If there's any pieces missing that you explicitly need, let Anima know what's missing so she can post it.
I deleted the attachment in comment 40... There are things in it that the letter from AOL Legal says had to be removed before it could be shared.
Created attachment 151559 [details] Main changes needed to apply the keyword chooser on create.html.tmpl
Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004. Anything flagged enhancement that hasn't already landed is being pushed out. If this bug is otherwise ready to land, we'll handle it on a case-by-case basis, please set the blocking2.20 flag to '?' if you think it qualifies.
The trunk is now frozen to prepare Bugzilla 2.22. Only bug fixes are accepted, no enhancement bugs. As this bug has a pretty low activity (especially from the assignee), it's retargetted to ---. If you want to work on it and you think you can have it fixed for 2.24, please retarget it accordingly (to 2.24).
Created attachment 261447 [details] [diff] [review] Complete patch, not quite finished This patch incorporates all the materials contributed by AOL along with some additional cleanup and some additional missing pieces retrieved from AOL that hadn't gotten posted. This *almost* works... problems that remain: 1) in Firefox, the chooser shows up at the top of the window instead of attached to the keywords field 2) in IE, the chooser shows up almost in the correct spot, but the list boxes are empty.
I'd appreciate some additional eyes on this since I'm out of ideas.
hmm, I forgot to add the hooks for show_bug.
Created attachment 261561 [details] [diff] [review] Complete patch, V1 You got bitten by same symptom as in bug 300743. You need to add px word after top and left values. Otherwise FF just logs in Error Console that it can't parse the value and ignores it. Here's an updated patch that should be complete. I tested it on both IE7 and Firefox. It has following changes: 1) Now positioning is cross browser compatible by using functions from js/util.js. This means this patch is now dependent on my patch in bug 182082. 2) I have no idea what that KeywordChooserObserver and IE bleeding stuff is. I can't see any bleeding and code doesn't work because chooserIEBleedOpenObserver (and probably also chooserIEBleedCloseObserver) are not defined. I just commented it out for this version but it should be removed unless somebody can fix it and proove that we need it. 3) Refactored the JS code in bug/create/create.html.tmpl into Keyword Chooser JS and template. This way the code doesn't need to be duplicated. I also removed duplicated and fixed buggy code so that now keywords are correctly loaded into the Keyword Chooser widget. 4) Keyword Chooser support is now available in enter_bug.cgi (incl. working when cloning and using keywords= parameter), show_bug.cgi, bug_list.cgi (change multiple), process_bug.cgi (for the bug shown after changes) and post_bug.cgi (it shows the submitted bug).
Comment on attachment 261561 [details] [diff] [review] Complete patch, V1 Wheee! Now time to get to the nitty gritty. :) (In reply to comment #51) > 1) Now positioning is cross browser compatible by using functions from > js/util.js. This means this patch is now dependent on my patch in bug 182082. Cool, the select-box handling code a the bottom of the keyword-chooser should probably go in util.js also. It came out of a general file equivalent to util.js in AOL's Bugzilla, and I just stuffed it at the bottom of keyword-chooser.js because nothing else was using it yet, and we didn't seem to have a util.js or equivalent yet. > 2) I have no idea what that KeywordChooserObserver and IE bleeding stuff is. > I can't see any bleeding and code doesn't work because > chooserIEBleedOpenObserver (and probably also chooserIEBleedCloseObserver) are > not defined. I just commented it out for this version but it should be removed > unless somebody can fix it and proove that we need it. That's something for caillon to answer, but I suspect it was a hack for IE 5.5 support (which was our minimum required browser on the IE side when we were working on this for AOL). There may also be another missing piece somewhere which defines those. If they turn out to be important, I can ask and try to get it. Seems to work okay without errors on IE6, but I don't have an IE5.5 to try. Maybe we don't care about IE5.5 nowadays. > 4) Keyword Chooser support is now available in enter_bug.cgi (incl. working > when cloning and using keywords= parameter), show_bug.cgi, bug_list.cgi > (change multiple), process_bug.cgi (for the bug shown after changes) and > post_bug.cgi (it shows the submitted bug). Coolness! Thanks a bunch! Only minor nit I have at this point is we probably need to change the colors. The css on this still contains the color scheme from AOL's Bugzilla. We should probably make the box the same blue we're using in the banner or something. r+ with that change though (pending an answer on IEBleed from caillon -- I poked him on that via IM, too, but he's away right now)
from caillon: ---- okay, from what i remember (digging way back), there was an issue with the opacity of the thing where it would cause the autocomplete dropdown to go behind the input control. I think there's probably a piece or something missing... but we can probably get by without it for now. guessing IE isn't that often used to report bugs ----
Created attachment 264763 [details] [diff] [review] Complete patch, V1.1 1) Moved (and documented as best I could) select handling functions to js/util.js as requested. 2) Killed off the IE bleeding control related code. We can file a new bug to fix this if it turns out there is a need for it after all. IE does have problems with bleeding as seen in bug 182082 for the help div control. 3) Changed keyword chooser border color to match title background. Carrying over r+ from justdave.
Comment on attachment 264763 [details] [diff] [review] Complete patch, V1.1 Doesn't adding to filterexceptions.pl require two reviews?
Comment on attachment 264763 [details] [diff] [review] Complete patch, V1.1 r=LpSolit for the filterexceptions.pl part.
Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.354; previous revision: 1.353 done Checking in enter_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v <-- enter_bug.cgi new revision: 1.156; previous revision: 1.155 done Checking in post_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v <-- post_bug.cgi new revision: 1.186; previous revision: 1.185 done Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.360; previous revision: 1.359 done Checking in show_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v <-- show_bug.cgi new revision: 1.51; previous revision: 1.50 done RCS file: /cvsroot/mozilla/webtools/bugzilla/js/keyword-chooser.js,v done Checking in js/keyword-chooser.js; /cvsroot/mozilla/webtools/bugzilla/js/keyword-chooser.js,v <-- keyword-chooser.js initial revision: 1.1 done Checking in js/util.js; /cvsroot/mozilla/webtools/bugzilla/js/util.js,v <-- util.js new revision: 1.2; previous revision: 1.1 done Checking in skins/standard/global.css; /cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v <-- global.css new revision: 1.36; previous revision: 1.35 done Checking in template/en/default/filterexceptions.pl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v <-- filterexceptions.pl new revision: 1.101; previous revision: 1.100 done Checking in template/en/default/bug/edit.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v <-- edit.html.tmpl new revision: 1.101; previous revision: 1.100 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/keyword-chooser.html.tmpl,v done Checking in template/en/default/bug/keyword-chooser.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/keyword-chooser.html.tmpl,v <-- keyword-chooser.html.tmpl initial revision: 1.1 done Checking in template/en/default/bug/show.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.html.tmpl,v <-- show.html.tmpl new revision: 1.18; previous revision: 1.17 done Checking in template/en/default/bug/create/create.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v <-- create.html.tmpl new revision: 1.77; previous revision: 1.76 done Checking in template/en/default/bug/create/created.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/created.html.tmpl,v <-- created.html.tmpl new revision: 1.10; previous revision: 1.9 done Checking in template/en/default/bug/process/header.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/header.html.tmpl,v <-- header.html.tmpl new revision: 1.6; previous revision: 1.5 done Checking in template/en/default/list/edit-multiple.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/list/edit-multiple.html.tmpl,v <-- edit-multiple.html.tmpl new revision: 1.42; previous revision: 1.41 done Checking in template/en/default/list/list.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.html.tmpl,v <-- list.html.tmpl new revision: 1.54; previous revision: 1.53 done
Added to the Bugzilla 3.2 release notes in bug 432331.
(In reply to comment #61) > Why am I not seeing this in 3.2-branch on landfill? Because it was removed in bug 452734 due to it receiving too much bad user feedback.