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.

Sign up for GitHub

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

Jump to bottom

First steps for issue #2024 - implemented the basic functionality of sound meter #2054

Merged
merged 2 commits into from
Feb 4, 2020
Merged

First steps for issue #2024 - implemented the basic functionality of sound meter #2054

merged 2 commits into from
Feb 4, 2020

Conversation

abhishekbisht1429
Copy link
Contributor

…sound meter

First steps for issues #2024

Changes:
Implemented the basic functionality of the sound meter. I am having some issues with smooth transition in the graph of the sound and also with the proper calibration of the sound meter.

Screenshot/s for the changes:
soundmeter_firststeps

Checklist:

  • I have used resources from strings.xml, dimens.xml and colors.xml without hard-coding them
  • No modifications done at the end of resource files strings.xml, dimens.xml or colors.xml
  • I have reformatted code in every file included in this PR [CTRL+ALT+L]
  • My code does not contain any extra lines or extra spaces
  • I have requested reviews from other members

APK for testing:
firststeps#issue2024.zip

@abhishekbisht1429 abhishekbisht1429 force-pushed the soundmeter branch 2 times, most recently from a88baa9 to ac5d599 Compare December 30, 2019 19:59
@abhishekbisht1429
Copy link
Contributor Author

@mariobehling I have implemented the basic functionality of a sound meter. Could you please have a look at it.
And also by using the butterknife annotations for binding the views, Codacy PR review is giving some negative feedback.

@abhishekbisht1429
Copy link
Contributor Author

abhishekbisht1429 commented Jan 4, 2020 •
edited
Loading

@neel1998 @adityastic @CloudyPadmal would love to have your opinion as well on this implementation so that I could complete it further.

CloudyPadmal
CloudyPadmal requested changes Jan 17, 2020
View reviewed changes
app/src/main/java/io/pslab/fragment/SoundMeterDataFragment.java
//TODO: To be implemented
}

/**********************************************************************************************
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use a consistent commenting style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! However i think, i did not completely understand what u meant by a consistent style. It would be really helpful if you could suggest me the type of style I should use for the further changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, I would say as in line 283. Also do not hardcode strings. Use them from strings.xml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay! you mean the This_ feature is yet to be implemented string right ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that part ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay cool, I will do the needful.

@CloudyPadmal CloudyPadmal added the Status: Review Required Requested reviews from peers and maintainers label Jan 17, 2020
@abhishekbisht1429
Copy link
Contributor Author

@CloudyPadmal also could you please suggest how should i proceed with this implementation ? It would be great to have some guidance. Like on the smooth transition of the graph part and the calibration of the sound meter.

@CloudyPadmal
Copy link
Collaborator

PR looks good. There seems to be some conflicted files @reckoner1429 . We can ignore the butterknife warnings in Codacy

@CloudyPadmal CloudyPadmal added the Status: Conflicts Merge conflicts blocking reviews and merges label Jan 18, 2020
@abhishekbisht1429
Copy link
Contributor Author

Okay, I will resolve the conflicts ASAP.

@abhishekbisht1429
Copy link
Contributor Author

abhishekbisht1429 commented Jan 18, 2020 •
edited
Loading

@CloudyPadmal I have resolved the conflict and also made the comments consistent to the best of my knowledge. Thanks :)

@mariobehling
Copy link
Member

Ok, unfortunately there were other PRs resulting in merge conflicts. Please help to finalize this and resolve the current conflicts.

@abhishekbisht1429
Copy link
Contributor Author

@mariobehling I have resolved the conflicts.

@abhishekbisht1429
Copy link
Contributor Author

@CloudyPadmal I have made the changes requested by you. Please have a look.

@CloudyPadmal CloudyPadmal merged commit e4d6322 into fossasia:development Feb 4, 2020
@CloudyPadmal CloudyPadmal added Feature New addition to the existing app and removed Status: Conflicts Merge conflicts blocking reviews and merges Status: Review Required Requested reviews from peers and maintainers labels Feb 4, 2020
@abhishekbisht1429 abhishekbisht1429 deleted the soundmeter branch February 5, 2020 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariobehling mariobehling mariobehling approved these changes

@CloudyPadmal CloudyPadmal CloudyPadmal approved these changes

Assignees

@abhishekbisht1429 abhishekbisht1429

Labels
Feature New addition to the existing app
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abhishekbisht1429 @CloudyPadmal @mariobehling