Skip to content

feat: extended record button to start sensor data fetching and recording #1305

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
Merged
merged 1 commit into from
Aug 11, 2018
Merged

feat: extended record button to start sensor data fetching and recording #1305

merged 1 commit into from
Aug 11, 2018

Conversation

@Avjeet
Copy link
Contributor

@Avjeet Avjeet commented Jul 29, 2018 •
edited
Loading

Fixes #1304

Changes:

  • Extended the record button to start the meter and graph plotting(i.e sensor data fetching) and also to record data.
  • Added new record button

To do
Store data in realm instead of CSV file.(That will be done in PR #1323 )

Screenshot/s for the changes:

Checklist: [Please tick following check boxes with [x] if the respective task is completed]

  • 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:
play.zip

@Avjeet Avjeet self-assigned this Jul 29, 2018
@Avjeet Avjeet added the In Progress Developer is working on the problem label Jul 29, 2018
@harsh-2711
Copy link
Contributor

harsh-2711 commented Jul 29, 2018

@Avjeet In my opened PR I have added play pause functionalities along with added functionality of deleting and exporting. So, I think it's better to go with one PR.

@Avjeet Avjeet changed the title feat: Added play/pause functionality feat: extended play/ pause functionality to start sensor data fetching Jul 29, 2018
@Avjeet
Copy link
Contributor Author

Avjeet commented Jul 29, 2018

@harsh-2711 Yeah I know that is why I added That your PR has to be merged first, in this PR I have extended that play-pause to even start the sensor data fetching(data change in graph and meter as you can see in the Screenshot) similar to phyfox that is why I created separate PR. The icons I have added are temporary And will remove after your PR is merged.

abhinavraj23
abhinavraj23 suggested changes Jul 31, 2018
View reviewed changes
Copy link
Member

@abhinavraj23 abhinavraj23 left a comment

Choose a reason for hiding this comment

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

@Avjeet Please resolve the merge conflicts then it's fine

@Avjeet Avjeet removed the In Progress Developer is working on the problem label Aug 2, 2018
@CloudyPadmal CloudyPadmal added Status: Review Required Requested reviews from peers and maintainers Status: Conflicts Merge conflicts blocking reviews and merges labels Aug 4, 2018
@CloudyPadmal
Copy link
Collaborator

CloudyPadmal commented Aug 7, 2018

@Avjeet please resolve the conflicts soon

@Avjeet Avjeet removed the Status: Conflicts Merge conflicts blocking reviews and merges label Aug 7, 2018
@cweitat cweitat added the Status: Conflicts Merge conflicts blocking reviews and merges label Aug 8, 2018
@Avjeet Avjeet force-pushed the Avi_play branch 2 times, most recently from 57902d4 to f586e2e Compare August 8, 2018 12:39
@Avjeet Avjeet changed the title feat: extended play/ pause functionality to start sensor data fetching feat: extended record button to start sensor data fetching and recording Aug 8, 2018
@Avjeet Avjeet removed the Status: Conflicts Merge conflicts blocking reviews and merges label Aug 8, 2018
@cweitat
Copy link
Contributor

cweitat commented Aug 8, 2018

@Avjeet clear up codacy thanks

@Avjeet
Copy link
Contributor Author

Avjeet commented Aug 8, 2018

@cweitat done with the changes

harsh-2711
harsh-2711 suggested changes Aug 8, 2018
View reviewed changes
Copy link
Contributor

@harsh-2711 harsh-2711 left a comment

Choose a reason for hiding this comment

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

@Avjeet Please change the icon to one that @mariobehling suggested in PR #1303

@harsh-2711
Copy link
Contributor

harsh-2711 commented Aug 8, 2018

@Avjeet This is the screen when I open the Lux Meter
screenshot_20180809-013148

Lux Meter isn't working. Please fix this issue. Also, the record icon has a different color than other icons.

@Avjeet
Copy link
Contributor Author

Avjeet commented Aug 8, 2018

@harsh-2711 The lux will only run when you click on the record button

@Avjeet
Copy link
Contributor Author

Avjeet commented Aug 8, 2018

icons updated

@Avjeet Avjeet requested a review from harsh-2711 August 8, 2018 21:56
@Avjeet Avjeet mentioned this pull request Aug 9, 2018
Merged
5 tasks
@cweitat
Copy link
Contributor

cweitat commented Aug 9, 2018

@Avjeet for the icons, can we have the same font size for both REC and STOP instead of squeezing the STOP text within the range? thanks

@Avjeet
Copy link
Contributor Author

Avjeet commented Aug 9, 2018

@cweitat Updated icons

@Avjeet
Copy link
Contributor Author

Avjeet commented Aug 9, 2018 •
edited
Loading

@CloudyPadmal @cweitat PLz review and merge it asap

@Avjeet Avjeet requested a review from cweitat August 9, 2018 13:13
@cweitat
Copy link
Contributor

cweitat commented Aug 9, 2018 •
edited
Loading

@Avjeet now REC is squashed and STOP looks normal?
Can't we have like this?

@Avjeet
Copy link
Contributor Author

Avjeet commented Aug 10, 2018 •
edited
Loading

@cweitat increased text size of rec in icon

@harsh-2711
Copy link
Contributor

harsh-2711 commented Aug 10, 2018

@Avjeet Even I was unable to figure out how to run Lux then how can we expect that users will understand? Also, the record and play button have a different meaning. If you are referencing PhyPhox, then they are using play button so that it is clear to the user. What do you think?

@Avjeet
Copy link
Contributor Author

Avjeet commented Aug 10, 2018 •
edited
Loading

@harsh-2711
This is done due to two reason:-

  1. The lux meter uses the sensor from the phone so if we kept the lux meter always running then there is the unnecessary usage of phone resources, having lux meter run only on record button resolve that issue.

  2. Second lux meter is a virtual electronic instrument and not a video recorder so I think it is clear for the user that the record button means to start recording data.
    Moreover if u feel we can always include that info in the bottom guide, which clears any confusion about that.

@cweitat
Copy link
Contributor

cweitat commented Aug 10, 2018

@Avjeet so both now look the same?

@Avjeet
added record functionality in lux meter
7002fa0 
changed icons

Increased icon text size

increased rec text size

some minor change
@Avjeet
Copy link
Contributor Author

Avjeet commented Aug 10, 2018 •
edited
Loading

@cweitat there was a minor change and yeah now both looks similar see the ss below:

@CloudyPadmal CloudyPadmal merged commit 898e461 into fossasia:development Aug 11, 2018
@CloudyPadmal CloudyPadmal removed the Status: Review Required Requested reviews from peers and maintainers label Aug 11, 2018
neel1998 pushed a commit to neel1998/pslab-android that referenced this pull request Jul 30, 2019
@Avjeet @CloudyPadmal
added record functionality in lux meter (fossasia#1305)
10baa82 
changed icons

Increased icon text size

increased rec text size

some minor change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cweitat cweitat cweitat approved these changes

@CloudyPadmal CloudyPadmal CloudyPadmal approved these changes

@abhinavraj23 abhinavraj23 Awaiting requested review from abhinavraj23

@harsh-2711 harsh-2711 Awaiting requested review from harsh-2711

Assignees

@Avjeet Avjeet

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Avjeet @harsh-2711 @CloudyPadmal @cweitat @abhinavraj23