Adium

Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#6293 closed enhancement (fixed)

Request for %_iTunes to not be case sensitive.

Reported by: tedger Owned by: evands
Milestone: Adium X 1.0.2 Component: AppleScript
Version: Severity: minor
Keywords: script itunes case senstive Cc: feedback@…
Patch Status:

Description

It's a minor change but apparently it's an annoying one. (I'm actually submitting this for my brother who uses the script).

Attachments (1)

iTunesCaseInsensitivityFix.diff (88.5 KB) - added by kiel 13 years ago.
Fix for this ticket.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 13 years ago by Trevin Ward

Type: defectenhancement

comment:2 Changed 13 years ago by Trevin Ward

Milestone: Adium X 1.0.1

Setting a milestone so people notice it and probably eventually set it to a better one.

comment:3 Changed 13 years ago by Chris Forsythe

Milestone: Adium X 1.0.1Needs dev review

comment:4 Changed 13 years ago by kiel

Why doesn't this work, anyone? The code reads as if it should. We're using a case insensitive compare...

comment:5 Changed 13 years ago by kiel

A work around for this issue would be to include %_iTunes and %_itunes in the phrase dictionary (see the iTunes plugin source). We shouldn't have to on the principle that a case insensitive compare of %_iTunes to %_itunes should evaluate to true.

comment:6 Changed 13 years ago by Evan Schoenberg

The case insensitive compare alone isn't enough; it uses objectForKey:trigger to lookup the substitution. Keys are case sensitive.

I think all the keys in the dict should be normalized to lowercase, and then the key on lookup should also be made lowercase.

comment:7 Changed 13 years ago by kiel

The way I understand it is if a message contains the text "I'm listening to %_itunes", the rangeOfString:options: method should find %_itunes and replace it with the appropriate replacement because %_iTunes is supposed to be the same as %_itunes when NSCaseInsensitveSearch is used.

Or do I have things the wrong way around?

comment:8 Changed 13 years ago by Evan Schoenberg

Look at ESiTunesPlugin.m#L372 in context of what I said above. That's the point at which a non-case-matched substitution attempt fails.

comment:9 Changed 13 years ago by kiel

The value of that pointer is @"%_iTunes" because the key from the phrase substitution dictionary is @"%_iTunes". The fix for this is using NSLiteralSearch | NSCaseInsensitiveSearch as an argument for the replaceOccurancesOfString:... method.

I have attached a patch that fixes the problem as I don't think I have commit privileges (if I still do, I've forgotten the command line witchery).

comment:10 Changed 13 years ago by kiel

Owner: changed from nobody to kiel

Changed 13 years ago by kiel

Fix for this ticket.

comment:11 in reply to:  9 Changed 13 years ago by evantest

Replying to kiel:

The value of that pointer is @"%_iTunes" because the key from the phrase substitution dictionary is @"%_iTunes". The fix for this is using NSLiteralSearch | NSCaseInsensitiveSearch as an argument for the replaceOccurancesOfString:... method.

Have you tested this patch? While that is actually a sane optimization change (no reason not to make it a literal search), I'm nearly 100% positive that that is neither the problem nor the solution. Changing the string upon which a search is performed isn't going to change what value is being passed to the dictionary for a lookup.

comment:12 Changed 13 years ago by Evan Schoenberg

(That was me - accidentally forgot to log out of a test account)

comment:13 Changed 13 years ago by kiel

field_haspatch: 01

The patch is tested and works.

If you look at all the code, no values are passed to the dictionary for lookup. When the plugin searches for a trigger, it enumerates through the keys of the substitution dictionaries and searches for the current key, case insensitively. So it will get @"%_iTunes" from the enumerator's nextObject method and finds @"%_itunes". This matches because we are using case insensitive search. While we've successfully identified the correct key, the problem was we where not replacing that key case insensitively which meant that @"%_itunes" is not replaced.

Therefore it doesn't matter what the case of the keys are in Adium or in the text to be filtered.

comment:14 in reply to:  13 Changed 13 years ago by kiel

Replying to kiel:

If you look at all the code, no values are passed to the dictionary for lookup

While this is actually an incorrect statement, it is true in the situation where we are trying to identify if a message contains a substitution.

comment:15 Changed 13 years ago by Eric Richie

Cc: feedback@… added

comment:16 Changed 13 years ago by Evan Schoenberg

Milestone: Needs dev reviewAdium X 1.0.2
Owner: changed from kiel to Evan Schoenberg
Status: newassigned

I see. I'd made the mistake of having already found a single solution in my mind and was expecting that solution... so when I looked at your patch I thought you were modifying the original search, not the replacement. Thanks for your patience :)

comment:17 Changed 13 years ago by Evan Schoenberg

Resolution: fixed
Status: assignedclosed

(In [19159]) Patch from kiel which makes the iTunes substitution triggers case insensitive. Fixes #6293. Thanks :)

comment:18 Changed 13 years ago by Evan Schoenberg

(In [19160]) Merged [19159]: Patch from kiel which makes the iTunes substitution triggers case insensitive. Fixes #6293. Thanks :)

Note: See TracTickets for help on using tickets.