Skip to content

Codereview #2 of 6 from 14#110

Open
GiovanniFurlan wants to merge 267 commits intodemo2from
TestFromLastImage
Open

Codereview #2 of 6 from 14#110
GiovanniFurlan wants to merge 267 commits intodemo2from
TestFromLastImage

Conversation

@GiovanniFurlan
Copy link
Copy Markdown
Collaborator

Hi Stefano Romanello,
i ask you to review my code please.
The code is in the file ManualTestOnSinglePhoto.
This class receive an angle by user. Then rotate the last image taken by this angle and use OCR on it. Then compare the text from normal and rotated photo and gave back similarity between text.
If you have some suggestion please tell me.
Thanks you.

mattia1142866 and others added 30 commits November 2, 2018 22:59
progetto di mattia molinaro - gruppo1
# Conflicts:
#	app/build.gradle
#	app/src/main/AndroidManifest.xml
#	app/src/main/java/unipd/se18/ocrcamera/CameraActivity.java
#	app/src/main/java/unipd/se18/ocrcamera/ResultActivity.java
#	app/src/main/java/unipd/se18/ocrcamera/TextExtractor.java
#	app/src/main/res/layout/activity_result.xml
#	build.gradle
… 'com.google.gms:google-services:4.0.1' because the TextExtractor implements that version of the library
Integrato internalStorageManager e aggiunto visualizzazione ultima foto + testo. Autore: Luca Moroldo
Some UI improvements. Smaller "take picture" button in CameraActivity. Added floating action button in result activity which takes back to the camera activity to take a new picture. Added NavigatorActivity which is the starting activity, if a picture is previously taken is starts result activity, if it isn't it start camera activity
Implements gyroscope rotation:
new class DeviceOrientation
new methods imageOrientation and rotateImage
start gyroscope on onCreate and onResume
stop gyroscope on onPause

Now OCR can read landscape, portrait, reverse landscape/portrait
PrandiniUniPD and others added 22 commits December 15, 2018 13:05
# Conflicts:
#	app/src/main/java/unipd/se18/ocrcamera/PhotoTester.java
#	app/src/main/java/unipd/se18/ocrcamera/ResultActivity.java
#	app/src/main/java/unipd/se18/ocrcamera/TestListener.java
#	app/src/main/res/values/strings.xml
Add ManualTestOnSinglePhoto class
Add a case in menu in result activity
-delete worker and InformationEntry class
-delete countDownLatch
-slim the code
-add input layout
-now test works on given value
Finished class and wrote last comments
Add test on class
Correction of hardcoding
String in strings.xml
Comments
* @param text string got from OCR
* @modify degreeTextView, confidenceTextView, warningTextView and foundTextView
*/
private void SetResult(String text){
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you were wrong to write but the methods should start with a lowercase letter.
Also I suggest to put the content of this method inside onTextRecognized, in this way all the UI contents are in the same place and you don't need to create 5 private objects for the UI in this class. What do you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes just distraction error

Text not correctly recognized
Start CameraActivity to take a new photo
*/
Intent i = new Intent(ManualTestOnSinglePhoto.this, CameraActivity.class);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to use a more correct name for this variable like cameraActivity

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right


final Bitmap photo = BitmapFactory.decodeFile(pathImage);

Button fab = findViewById(R.id.btnConfirm);
Copy link
Copy Markdown
Collaborator

@stefanoromanello stefanoromanello Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest a better name for this button object like buttonAction or a better name for describe the object

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's the only button but anyway this would be better

final int toleranceValue=4;


if (text.equals("")) {
Copy link
Copy Markdown
Collaborator

@stefanoromanello stefanoromanello Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to implement a warning but maybe I understood wrongly this part. Why are you only sending the warning if the text extracted is less than 30? Also why are you using these 3 bounds? Shouldn't be better to do like (length-text.lenght) and get the length difference? In this way I can understand if I get more or fewer letters. If the result is positive I've lost letters, If its negative I've obtained more letters.
Because I don't see the utility to know if the new extracted text is less than 30 or 20 characters, with this class I want to know if the rotation improves or aggravate the image recognition.
Tell me what you think

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your solution is good, i would think about this new implementation

* @param length int of length of starting string
* @return warnings
*/
private String findWarning(java.lang.String text, int length) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to change the name of these 2 parameters. A possible solution could be newExtractedText and originalTextLength

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this good advice

/**
* UI
*/
private EditText degreeEditText;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

degreeEditText is used only in onCreate. Is there a reason to keep it outside the onCreate?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to have all the variable at the beginning of the class. Do u think it's better that way or create a new variable in the method where it is used, if it's used only in this method?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to create a variable inside the method if its used only there



//Get image path and text of the last image from preferences
SharedPreferences prefs = this.getSharedPreferences("prefs", MODE_PRIVATE);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to add the name of the SharedPreferences to the xml resource for the Strings

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes thanks for the advice

final correction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.