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

Cleaned up SmsDetector.java implementation, related issues #615, #537 #635

Merged
merged 1 commit into from
Nov 20, 2015
Merged

Conversation

smarek
Copy link
Member

@smarek smarek commented Nov 14, 2015

So, what was done:

  • Removed unused class-global fields
  • Removed, WTF, code new Thread().sleep(500);
  • Rewritten implementation of reading String lines out of buffer, including one more local temporary variable
  • Fixed author javadoc tag
  • Replaced first level of for-loop in checkForSilentSms with foreach
  • Removed pointless calls toString() in context like this DETECTION_PHONENUM_SMS_DATA[2].toString() (where DETECTION_PHONENUM_SMS_DATA is array of Strings)
    Some encoding sincerely occured on-the-fly, hope you don't mind.
    SmsDetector doesnt check how many bytes where read from input #615 discussion points are imho fixed by this PR
    Detection of Silent SMS (part 2) #537 pointed out that the problem with performance is in string retrieval/parsing/checking, I agree and we should imho migrate the code over to regexp declarations, and I'm worried we do not now correctly handle situations on border of string stream (where detection would fail because the data are incorrectly splitted by newline character)

@ghost
Copy link

ghost commented Nov 14, 2015

@smarek Thanks for modifying code and improving something similar to what my last PR was going to do. Nice to see someone helping out on detection because this is what the app is about.

@ghost
Copy link

ghost commented Nov 14, 2015

Also so from tests I done regexp seems very slow a bit like string format.

@SecUpwN
Copy link
Member

SecUpwN commented Nov 14, 2015

I feel blessed to see you repsoning here, @banjaxbanjo. Since you've closed your important pull requests, do you think you could add your changes back again after I've merged this on sunday?

@ghost
Copy link

ghost commented Nov 14, 2015

@SecUpwN I will have a look at my offline dev sometime this week with original PR to see what can be merged. No promises😉

@SecUpwN
Copy link
Member

SecUpwN commented Nov 15, 2015

No promises

No need to promise anything, I hate promises but love honesty. ;-) Shall I merge the proposed changes?

@ghost
Copy link

ghost commented Nov 15, 2015

The offline class I have for sms detection is copy & paste the full class and uses a different method for reading buffer, Just needs minor formatting and its 10 times better than current class, be no problem to merge this PR by smarek but if I was to add the previous PR I would be overwriting this pr for it to work the way I intended. Can work behind scenes on class with smarek so we can finish this class for good.

@SecUpwN
Copy link
Member

SecUpwN commented Nov 15, 2015

Can work behind scenes on class with smarek so we can finish this class for good.

Would you really do that? This would be absolutely lovely! I guess @smarek can add the necessary changes to this PR so that it gets polished and does not have to be closed. Thanks and happy hacking! :)

@smarek
Copy link
Member Author

smarek commented Nov 15, 2015

@SecUpwN will try

@banjaxbanjo please push your code somewhere online and let me know, i'll be happy to assist you to rebase it on top of latest dev (or maybe merge it with changes proposed by this PR).

@smarek
Copy link
Member Author

smarek commented Nov 15, 2015

@smarek
Copy link
Member Author

smarek commented Nov 15, 2015

So I've rebased the original PR#619 into second commit, to be clear about what has changed.
I've also refactored the code, fixed names in whole class, formatted the code, etc.

Please note, it's possible I've broken something with this rebase, so I'd be glad for testing from original author @banjaxbanjo and code review from anyone that's interested.

@smarek
Copy link
Member Author

smarek commented Nov 15, 2015

Also I've taken some of the feedback @agilob provided to original PR, and integrated it within (checkForSms method rewritten for optimized usage of .split call, handled possible incorrect detection string and LOADED_DETECTION_STRINGS finalized)

@SecUpwN
Copy link
Member

SecUpwN commented Nov 15, 2015

Huge thanks to both of you @smarek and @banjaxbanjo for working behind the scenes to improve that important part of our code. I'll be waiting for @banjaxbanjo's green light here to merge this next week.

@ghost
Copy link

ghost commented Nov 16, 2015

@SecUpwN this is working smooth now please merge. Cheers @marek awesome merge job and nice coding.

@smarek
Copy link
Member Author

smarek commented Nov 16, 2015

Should I squash the commits so we don't spam the repo history?

@smarek
Copy link
Member Author

smarek commented Nov 16, 2015

Well I squashed them together, @banjaxbanjo thanks for testing and support in development
@SecUpwN now it's ready to be merged 👍

Merged with PR#619 proposal, fixed up looping, buffering, cleaned up names and calls, to be more clear in code
@smarek
Copy link
Member Author

smarek commented Nov 16, 2015

@agilob thanks for feedback, is everything OK now? Give us please a your status, so we can merge it.

@ghost
Copy link

ghost commented Nov 16, 2015

@smarek Cheers for all the mods👍

@smarek smarek self-assigned this Nov 17, 2015
@smarek smarek added this to the v0.1.37-alpha milestone Nov 17, 2015
@SecUpwN
Copy link
Member

SecUpwN commented Nov 17, 2015

Waiting for the green light to merge this stunning work. 💚

@smarek
Copy link
Member Author

smarek commented Nov 19, 2015

@SecUpwN I think we're good to go here, all @agilob comments were addressed in refactoring the commit, would be nice to end up with working sms detection again 👍

@SecUpwN
Copy link
Member

SecUpwN commented Nov 19, 2015

...were addressed in refactoring the commit...

So is this the point were I should merge, or shall I wait? ;-)

@smarek
Copy link
Member Author

smarek commented Nov 19, 2015

I think we can merge it 😈

SecUpwN added a commit that referenced this pull request Nov 20, 2015
Cleaned up SmsDetector.java implementation, related issues #615, #537
@SecUpwN SecUpwN merged commit 0d5c369 into CellularPrivacy:development Nov 20, 2015
@smarek smarek deleted the class-cleanup-2 branch November 20, 2015 08:54
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

2 participants