It's going to be very painful to change custom keyword tokens (the %s) things at *any* point. The sooner it's done, the less pain will be caused, as fewer folks will be using the feature. If bug 124237 and other conceivable custom keyword query improvements are to have any chance of being possible in the future, the %s token needs to be future-proofed to allow for future expansion. I propose that %s be eliminated in favor of %s; (note the addition of the semicolon). -matt

huh?

see comments thirteen and fourteen from http://bugzilla.mozilla.org/show_bug.cgi?id=124237#c13 Basically, if custom keywords are going to has any chance of being more usefull in the future, they'll need a syntax which leaves room for expansion. The current %s syntax is not robust in this sense becuase you can't tell when an extended %s token ends. In the future, we'll either have to change the %s syntax (breaking lots of exsiting stuff), or we'll have to support two syntaxes... one useing %s and one (which works better) using %q or something. In the later case, two systems then need to be maintained. What a bitch. :/ In the former case, we can gain most of the future-proofing required by having the %s token be %s; instead (note addition of semicolon). This will break things for folks, but can be release-noted and dealt with much more pleasantly at this point than after 1.0 -matt

Would a patch be welcome for this? Or is this just not wanted?

my *impression* is that there is never a time when patches are "not wanted". If you attach a patch, then either reviewers/drivers will like it enough to fold in to mozilla, or maybe other folks will fold it into their own mozilla-based products. Either way, if the patch is there, folks have the opportunity to decide. If it's not there, then they don't :) My understanding is that this is not high on any engineers' lists of priorities. They've got bigger fish to fry right now. However, if you were to come up with a patch, they're probably more likely to have time to review the patch than to come up with one themselves. So, I'd say "please! feel free to patch" -matt (who is obviously biased, having created this bug)

This patch will change "%s" to "%s;".

Matthew Wickline wrote [in an email]: > just got word from Asa Dotzler: > Asa> Thanks for the note Matt. > Asa> This looks like good post-1.0 work. Asa, are you sure you didn't misunderstand this bug? What you wrote to Matt sounds as if you thought this bug was about *implementing* multiple substitutions, which it's not. It's just about changing the "%s" token to "%s;" so that it will be possible to expand the syntax in the future. When multiple word substitution is implemented, it will need a syntax like "%s1;", "%s2;", etc. When that happens, all existing bookmark queries which use "%s" will have to be changed, since it won't be possible to tell this: "%s1;" -- the "1;" is part of the keyword token for multiple word substitution. from this: "%s1;" -- the "1;" is actually part of the URL and only the "%s" part should be replaced. Changing the token to "%s;" solves that forward-compatibility problem. This bug suggests that it would be better to change the token ASAP and *before* we reach 1.0, so that the amount of keyworded bookmarks in need of updating will be minimal, and so that we don't unnecessarily break compatibility between 1.x.x releases.

I'm not a C programmer, and this "patch" was created by hand-editing the patch 74320, so may not even work. The idea is to throw a while loop in there to also fix bug 98749 ("Bookmark keyword substitution only work on the first token"). However, the code I've suggested has a /fatal/ flaw. If the inserted text has a "%s;" then the code will loop forever filling up memeory with a growing string until Something Bad happens. I don't know enough to implement the fix, but I'm hoping someone else will. We need to get *all* the indices of "%s;" at the outset prior to any substitution. If there's a single command to do that, great. If not, a loop could do it. Then we need to substitute from the rightmost case (so that the indices to the left will still be correct values) and replace all of the "%s;" instances in the original. Since we go right to left and never look at the inserted stuff on the right, it would no longer matter if the inserted stuff contained "%s;" This code could be re-used in many places, I'm thinking, as it gives you the equivilant of a perl s///g which I personally find more usefull than most of my toes! So, if there isn't already an encapulated method for this, I'd suggest wrapping it up an adding it to whatever base string classes mozilla uses. Having a global substitution method also gets you that much closer to being able to implement the build_uri subroutine attached to bug 124237 and I'm expecting would be Potentially Usefull in many other places. Maybe there's already such a function or method available. In that case, this would be a good time to use it and fix two bugs with one patch. Better to relnote this %s; business in 1.0 than 1.x :) -matt

Whoops! I meant to have aOffset = shortcutURL.indexOf("%s;"); in that loop of course. -matt

Duh! it's ecmascript, not C. OK, well, I'm still a perl guy not an ecmascript guy, but how about this attempt? It uses split and join, but in a perlish way. I don't know if they work the same way in ecmascript or not. I'm going to stop checking the "patch" checkbox as these are hand edits of a previous patch and I don't know if they'll apply cleanly. -matt

You can test your patches with Patch Maker: http://www.gerv.net/software/patch-maker/build-mode.html

Tried using Patch Maker. I'm on Mac OS X. In the instructions, I got to > Execute pma content/navigator/navigator.xul > (flip the slashes on Windows) to add > navigator.xul to your patch. and failed. I can find no path looking like that. I thought maybe the instructions were old, and poked around looking for a likely contemporary equivilant. No dice. I also tried looking for xpfe/browser/resources/content/navigator.js but no luck there as well. FWIW, here's what I have in chrome/ [localhost:Contents/MacOS/chrome] wickline% pwd /Applications/Mozilla/Mozilla.app/Contents/MacOS/chrome [localhost:Contents/MacOS/chrome] wickline% ls US help US.jar help.jar b inspector chatzilla inspector.jar chatzilla.jar installed-chrome.txt chrome.rdf installed-chrome.txt.orig classic messenger classic.jar messenger.jar comm modern comm.jar modern.jar content-packs overlayinfo content-packs.jar pipnss en-US pipnss.jar en-US.jar pippki en-mac pippki.jar en-mac.jar toolkit en-unix.jar toolkit.jar en-win.jar venkman forms venkman.jar forms.jar Maybe I'm in the wrong chrome directory? I couldn't find a more likely looking place, but I probably just don't know where to look. Any suggestions? Ideally someone familiar with Mac OS X instals could suggest where to look. I *did* poke around in some of the directories, but without luck. -matt

Please ask in the newsgroups. We are getting *way* off-topic here, and I feel very bad for spamming poor Asa whom I added to the CC list when I asked him if he might have misunderstood this bug.

did me some ecmascript learnin' and found out how to do a global substitution in one line. This should fix bug 12420 (this bug) as well as bug 98749.

marking nsbeta1 ADT, drivers, et al: This change to internet keywords will need to happen for keyword queries to get smarter in the future. We have (as I see it) three mutually exclusive options... 1) Apply a two-line patch *Now* and relnote the difference. Users with custom keyword queries need to update at 1.0. Some percent of users won't read the release notes, and will think mozilla is broken. The percent will be small as most custom keyword query users at this point are mozilla geeks. 2) Apply this two-line patch after the mozilla 1.0 release. Many many many more users need to update their bookmarks whenever this change happens. Most users won't read the release notes because 1.0 and beyond are going to be used by way more folks than mozilla geeks. Whenever this patch gets applied post 1.0, they'll think things broke. 3) Never apply this patch. When we add more features to the custom keyword functionality we need a second interface, and some way for users to toggle between them. This is not a good idea. It means two bunches of code to maintain plus code to coordinate the two URI template mechanisms. In other words, Pick one of these: 1: hurt a few users now, before 1.0 2: hurt many users later, after 1.0 3: hurt developers for the duration of the code I think that option #1 is the best for mozilla, and hope this *very* simple patch can make it into 1.0. Please Please Please :) Before 1.0 is exactly when changes of this sort are supposed to happen. For some idea of the features which this change will allow in the future, see RFE bug 124237. I've already implemented that feature in perl, and if I get enough tuits I may do so in ecmascript. That RFE gives folks the ability to use near natural langage in their URL Bar if they have smart enough URI Templates in their bookmarks file. If implemented, I expect web service providers will provide those smart URI templates, and mozilla users will be typing all sorts of spiffy queries right in their URL bars. -matt PS: I've been informed that it's ok for anyone to nsbeta1, and netscape folks then plus or minus that upon triage. If that isn't so, please correct me, and I apologize for the error. (Note that just removing nsbeta1 won't correct me, as I'll wonder whether it was removed because I shouldn't have put it there... or because it was triaged as not significant.

nsbeta1- per Nav triage team

Since this will not happen for 1.0, we will have to keep supporting the "%s" syntax. --> WONTFIX

since asa indicated that it was good post 1.0 work, if we leave this wontfix, then we need a new bug for doing it some time after 1.0. removing wontfix and changing summary seems more economical. -matt

I'm pretty sure he meant that *multiple keyword substitutions* is good post-1.0 work, not that *changing the syntax* is. The syntax SHOULD NOT be changed after 1.0 has been released. If it MUST happen, it SHALL be covered by the patch which implements multiple keyword substitutions.

.

> I'm pretty sure he meant that *multiple keyword substitutions* is good post-1.0 I suppose asa can best say what he meant... but he did add the "mozilla 1.0-" keyword to this bug when it had no keywords at all -matt

> I suppose asa can best say what he meant... He can, but he is a very busy man. :-) > but he did add the "mozilla 1.0-" keyword to this bug when it had no keywords at > all Yes, but my point is that there is no reason for keeping this bug open if it won't happen for 1.0 since we already have a bug about allowing multiple substitutions. So what do we need this one for?

> there is no reason for keeping this bug open if it > won't happen for 1.0 since we already have a bug > about allowing multiple substitutions. > So what do we need this one for? because this bug is not about multiple substitutions. It is about terminated tokens. Someone could fix the multiple substitutions without fixing this one. They are two separate bugs. It just happens to be the case that the patch attached to this one fixes that one as a bonus. That doesn't mean that this is a dup of that. -matt

> They are two separate bugs. It just happens to be the case that the patch > attached to this one fixes that one as a bonus. Ah, you are talking about bug 98749? Then it makes sense. I was talking about bug 124237. :-)

I see :) I'm putting bug 124237 as dependent on this one. It could end up that they get fixed simultaneously, but it could also happen that this one gets fixed first. I still have some hope that some driver will notice the simple patch here and slide it into 1.0 while killing a spare 30 seconds before a meeting or something. -matt

if they aren't fixed by 1.0, then hopefully soon after...

I don't see why we need this now. Later, we can support a different syntax altogether for multiple/positional parameters. However, if you get r= and sr= from the relevant owner (ben) and a super-reviewer, you can propose this for 1.0. No driver is going to spend time on it (more than I have, anyway :-) without r= and sr= on the patch. Breaking longstanding uses of %s seems like a bad idea to me, however -- I still believe the best way to extend things is compatibly, before or after 1.0. That means leaving %s alone and using {1}, {2}, etc. or some other such scheme. /be

Mass reassign of my non-Firefox bugs to ben_seamonkey@hotmail.com

MASS-CHANGE: This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state. If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way. If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar). If no action happens within the next few months, we move this bug report to an EXPIRED state. Query tag for this change: mass-UNCONFIRM-20090614

MASS-CHANGE: This bug report is registered in the SeaMonkey product, but still has no comment since the inception of the SeaMonkey project 5 years ago. Because of this, we're resolving the bug as EXPIRED. If you still can reproduce the bug on SeaMonkey 2 or otherwise think it's still valid, please REOPEN it and if it is a platform or toolkit issue, move it to the according component. Query tag for this change: EXPIRED-20100420