-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add autocomplete fields to TransactionViewActivity #152
base: master
Are you sure you want to change the base?
Conversation
Sometimes that the best reason for doing it. (: I've been thinking about this today and ran some experiments. Let me share some some of my thoughts. Before I wrote this application I was using a closed source application which did some of the same things. The basic idea was the same, budgets and transactions. One feature that app had which mine does not is autocomplete. One part of autocomplete that bothered me is if i entered something spelled incorrectly once, it would always appear in the autocomplete list from there on out. It would always remind me of that time I typed it wrong. Though, in that case, I think if i were to edit the transaction in question it would correct itself. I think that app would gather the hints from the values in the database, rather than keeping a separate list. Reviewing the proposed change, my understanding is that it keeps a set of Strings for all descriptions, notes, values, and accounts, which is parallel to the data in the database. In addition, the sets will grow unbounded; the more unique entries added the more options added to the autocomplete. One thing that the change does not account for is how to restore that list if one uses the backup/restore option for the app. Here are the four areas which I think need to be addressed for this change to be accepted.
Following are details.
Besides these three areas, there are a few smaller nit-pick areas. One is {} usage. I like putting the { on the next line instead of inline with the if. I was hoping to not wall-of-text you, so sorry about that. If you are still interested in contributing this feature and would like some advice on the details (you mentioned you are not normally an Android dev), let me know; I'll be glad to point you in the right direction. |
Hey, I appreciate your thorough thoughts! I'm not married to the idea of using
Like I mentioned above, I agree with this and the benefits are pretty convincing, I hadn't considered typos at all to be completely honest.
Not sure about how to handle this one. The last ones used could be a good idea. I was also thinking querying for the most used ones instead of the last ones. What do you think of that? As for the number of entries, that's up to you.
Agreed. Just realized I misread the original issue.
This I'm gonna have to check out the rest of the test to get an idea out of, I haven't worked with that test framework but I'll work on those tests.
Haha, you got it! And yeah, I'd appreciate the input along the way and you bet I'm still interested, feedback like this on PRs is motivating so thanks! |
For the first pass we could try listing all options from the database. From there it can be trimmed down. Depending on the performance impact, we can see if querying the most recent or the most frequent is the better option. I'd think that having the most recently used may be valuable, otherwise it will take many many entries until something finally appears in the autocomplete list. Maybe there is a balance. |
Trying to meet the needs presented in #110
Not usually an Android dev, but figured I'd contribute anyway since this is a feature I really wanted myself.