Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added method to set the character that triggers token creation #34

Merged
merged 11 commits into from Jan 7, 2015

Conversation

wdullaer
Copy link
Contributor

I wanted the EditTextView to create tokens when the user inputs a space, so I removed the hardcoded comparison with ',' and added some convenience methods to set the character that triggers token creation.
It is possible to set multiple characters.

The code can be shorter by using a List instead of an array of chars, but given that char is a primitive I think the current approach is less resource intensive.

I hope I did this pull request thing right, I'm quite new to GitHub

@mgod
Copy link
Contributor

mgod commented Jan 12, 2014

@wdullaer The pull request is correct. Unfortunately, there are a couple of issue with the patch. I've hardcoded ',' at a number of spots in the code beyond what you've looked at. There's also the MultiAutoCompleteTextView.CommaTokenizer I use to parse and handle the content behind the token in places. To see some of the weird behavior, boot up a 2.3.3 simulator or device, set the splitChar to space and type "ter,ter ".

I also use " " as a separator that shouldn't complete in come cases, mostly because I need a character the user can delete that doesn't show up visually to handle some odd cases when deleting.

I suspect getting this all correct for the general case is going to be harder than it's worth and you may want to just get the subset of behaviors you need working in your case, though I strongly urge you to test for weird interactions on 2.3.3, 4.0, 4.2.2. If you do want to get a more comprehensive version working, I would look at factoring out some of the behavior into an AutoCompleteTextView.Tokenizer subclass and see if you can get a more reliable results by working with your own custom subclass there.

@wdullaer
Copy link
Contributor Author

Hi, I've refined my patch a bit.
I've made all your hardcoded ',' reference the splitChar array. In the case where you are adding a splitChar I've tried to make sure that " " is not used.

I have also added a custom Tokenizer class. It borrows most of its code from the CommaTokenizer, but you can configure the token characters in the same way as the splitCharacters. It should behave identical to CommaTokenizer.
The code makes sure that the splitCharacters are also considered as tokens in the Tokenizer. (Your code would also breakdown if you insert a custom Tokenizer using setTokenizer that does not consider ',' to be a token). This should eliminate most of the weird behaviour.

I haven't run into any major issues so far, then again, I haven't really done any extensive testing yet.
If there are big issues when using " " as a splitChar it might be worthwhile to see if you can use a tab character as an invisible separator that doesn't complete. These are also not visible, but much harder to type on soft keyboards.

It would be cool if you could take another look and let me know if you like the way I'm going with this and if you would eventually consider merging it. (If not, I'll probably just test for my personal use cases and call it a day).

@mgod
Copy link
Contributor

mgod commented Jan 22, 2014

@wdullaer This is exactly what I had in mind. There are a few tweaks I'll want to make, so I'll probably merge this when I have some spare cycles to work on the code.

@wdullaer
Copy link
Contributor Author

Great news. Your library saved me a lot of work, so I'm happy I could contribute back.
I have a separate branch called cardnote where I've done some other tweaks/fixes unrelated to these. (There was an inflate exception when trying to create a token from an empty String).

I'm also going to see if I can trigger the token creation when the view loses focus (gmail does this).

If you can describe what you'd want changed in my implementation (maybe with some short comments on the code), I could try to implement those for you.

@vivekanandg
Copy link

  • edittext view is broken with 'space' as split char
  • gives 'java.lang.IndexOutOfBoundsException' at a few places
    i.)com.tokenautocomplete.TokenCompleteTextView.handleFocus(TokenCompleteTextView.java:505)
    ii.)com.tokenautocomplete.TokenCompleteTextView.onSelectionChanged(TokenCompleteTextView.java:471)

@vivekanandg
Copy link

crashes the app onLongClick

@wdullaer
Copy link
Contributor Author

Is there any chance that this pull request will ever be merged? Most of the issues I have fixed are still present in the current version.

I recently started working ont this library again to fix a few niggles I had with it. If there is no more chance that my changes can be merged here, I'm going to change the code structure. The current one makes it hard to maintain for me.

@mgod
Copy link
Contributor

mgod commented Jan 5, 2015

@wdullaer I'm going to commit to merging this week. I'm sorry for the super slow process on this one.

@mgod
Copy link
Contributor

mgod commented Jan 5, 2015

Also, if the structure changes are not project-specific, I'd love to hear about what you're having trouble with.

@wdullaer
Copy link
Contributor Author

wdullaer commented Jan 5, 2015

Hi Marshall,
I was getting a bit annoyed by the line length of the file, so I wanted to see if I could split of some of the private inner classes into separate files for ease of maintenance.
I haven't merged my latest changes into this branch. I happen to have some time for that tomorrow, so you should find something workable here tomorrow.

@wdullaer
Copy link
Contributor Author

wdullaer commented Jan 7, 2015

I've created a new pull request #90
That way I don't pollute my master branch too much should you chose not to merge my changes.

@mgod mgod merged commit f9c6fea into splitwise:master Jan 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants