Skip to content

Conversation

@abhinavraj23
Copy link
Member

@abhinavraj23 abhinavraj23 commented Aug 8, 2018

Fixes #1337

Changes:
Added the alert-box for pressure sensor in barometer and changed the file name from Barometer_activity to BarometerActivity

Screenshot/s for the changes: [Add screenshot/s of the layout where you made changes or a *.gif containing a demonstration]

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: [Compress the app-debug.apk file into a <feature>.rar or <feature>.zip file and upload it here]
barometer.apk.zip

@abhinavraj23 abhinavraj23 requested review from Avjeet, CloudyPadmal and harsh-2711 and removed request for CloudyPadmal August 8, 2018 18:55
@abhinavraj23 abhinavraj23 self-assigned this Aug 8, 2018
@abhinavraj23 abhinavraj23 added the Status: Review Required Requested reviews from peers and maintainers label Aug 8, 2018
@abhinavraj23 abhinavraj23 changed the title Created alert box for barometer and changed the file name feat: created alert box for barometer and changed the file name Aug 8, 2018
<string name="barometer_description">Measures the pressure of the atmosphere</string>
<string name="barometer">Barometer</string>
<string name="barometer_alert_title">Sensor Not Present</string>
<string name="barometer_alert_description">Atmospheric pressure sensor is not present in your device, you need to use external sensor with PSLab for barometer</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we use A barometer instead of Atmospheric pressure sensor?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have external sensor with PSLab for barometer to work. instead of external sensor with PSLab for barometer?

.setPositiveButton("ok", new DialogInterface.OnClickListener() {
@Override
public void onClick(DialogInterface dialog, int which) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also do some refactoring here

@cweitat cweitat added the Status: Conflicts Merge conflicts blocking reviews and merges label Aug 9, 2018
<string name="barometer_description">Measures the pressure of the atmosphere</string>
<string name="barometer">Barometer</string>
<string name="barometer_alert_title">Sensor Not Present</string>
<string name="barometer_alert_description">Atmospheric pressure sensor is not present in your device, you need to use external sensor with PSLab for barometer</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have external sensor with PSLab for barometer to work. instead of external sensor with PSLab for barometer?

@abhinavraj23
Copy link
Member Author

@CloudyPadmal Changes done 👍 , @yatri1609 I think mentioning PSLab will make sense to the users. What do you think about this @CloudyPadmal .

@abhinavraj23 abhinavraj23 removed the Status: Conflicts Merge conflicts blocking reviews and merges label Aug 9, 2018
@abhinavraj23
Copy link
Member Author

@CloudyPadmal @harsh-2711 @Avjeet @yatri1609 Please review


<string name="barometer">Barometer</string>
<string name="barometer_alert_title">Sensor Not Present</string>
<string name="barometer_alert_description">Barometer sensor is not present in your device, you need to use external sensor with PSLab for barometer</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about;

you need to use an external sensor (BMP180) with PSLab to use this instrument

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think it makes more sense.

@abhinavraj23
Copy link
Member Author

@CloudyPadmal Changes done 👍

@abhinavraj23
Copy link
Member Author

@CloudyPadmal @harsh-2711 @Avjeet Please review asap


<string name="barometer">Barometer</string>
<string name="barometer_alert_title">Sensor Not Present</string>
<string name="barometer_alert_description">Barometer sensor is not present in your device, you need to use external sensor(BMP180) with PSLab for barometer</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

This line isn't proper. You could rather use
"Barometer sensor is not present in your device. Please connect an external sensor(BMP180) with the PSLab I2C pin for using Barometer instrument"

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure @harsh-2711 , I will make the change:+1

@abhinavraj23
Copy link
Member Author

@harsh-2711 Changes done, please review 👍

@cweitat
Copy link
Contributor

cweitat commented Aug 11, 2018

@Avjeet Too lengthy.

No barometer sensor is detected. -line break- Connect an external sensor (BMP180) to I2C pin on PSLab..

Centralise and justify the text

@cweitat
Copy link
Contributor

cweitat commented Aug 11, 2018

@abhinavraj23 also, what's the priority for this instrument? If both phone and PSLab have the sensors, from which device will it take the readings from?

@abhinavraj23
Copy link
Member Author

abhinavraj23 commented Aug 11, 2018

@cweitat currently it is only implemented by mobile sensors and and not through external sensor as there are issues in i2c communication library. But very few devices have pressure sensors(it is not common) so we should add the alert-box as the user can get confuse

@cweitat
Copy link
Contributor

cweitat commented Aug 11, 2018

@abhinavraj23 so which means by default the alert box will appear?
If by default the alert box will appear then change the message to just

Please connect an external sensor (BMP180) to I2C pin on PSLab.

Because if the phone has the sensor then this alert box won't show. This alert box only shown when no sensor is found.

@abhinavraj23
Copy link
Member Author

abhinavraj23 commented Aug 11, 2018

In those devices in which the mobile sensor is not present ( i.e in most of the devices). But not adding the alert-box will make an impression on the users that the app itself is not working, which is not true as the app is working fine for the mobile sensors.

@cweitat
Copy link
Contributor

cweitat commented Aug 11, 2018

@abhinavraj23 I'm not saying to remove the alert box. I'm saying that the alert box won't show if there is a mobile or external sensor detected.

@abhinavraj23
Copy link
Member Author

@cweitat if we just display

Please connect an external sensor (BMP180) to I2C pin on PSLab.

Then the user will not get the understanding that the mobile sensor is not present in his device. Will that be okay?

@cweitat
Copy link
Contributor

cweitat commented Aug 11, 2018

Then this

No barometer sensor in device. -line break- Connect an sensor (BMP180) to I2C pin on PSLab..

@abhinavraj23
Copy link
Member Author

@cweitat Changes done please review 👍

@abhinavraj23
Copy link
Member Author

@cweitat
Copy link
Contributor

cweitat commented Aug 12, 2018

Justify the text
Text change to

Connect an BMP180 sensor to I2C pin on PSLab to use this instrument.

@abhinavraj23
Copy link
Member Author

Changes done @cweitat. Please review 👍


<string name="barometer">Barometer</string>
<string name="barometer_alert_title">No barometer sensor in device</string>
<string name="barometer_alert_description">Connect a BMP180 sensor to I2C pin on PSLab to use this instrument</string>
Copy link
Collaborator

@CloudyPadmal CloudyPadmal Aug 12, 2018

Choose a reason for hiding this comment

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

Connect a BMP180 sensor using I2C pins on PSLab to use this instrument

@abhinavraj23
Copy link
Member Author

@CloudyPadmal changes done please review 👍

Copy link
Collaborator

@CloudyPadmal CloudyPadmal left a comment

Choose a reason for hiding this comment

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

Nice

@abhinavraj23 abhinavraj23 merged commit 6c81563 into fossasia:development Aug 12, 2018
@abhinavraj23 abhinavraj23 deleted the barometer_alertbox branch October 1, 2018 06:51
@CloudyPadmal CloudyPadmal removed the Status: Review Required Requested reviews from peers and maintainers label Dec 13, 2018
neel1998 pushed a commit to neel1998/pslab-android that referenced this pull request Jul 30, 2019
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.

5 participants