search

e-mission / e-mission-docs

Repository for docs and issues. If you need help, please file an issue here. Public conversations are better for open source projects than private email.
https://e-mission.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
15 stars 34 forks source link

💄 Finish rewriting diary/service implementation #932

Closed shankari closed 1 year ago

shankari commented 1 year ago

Tracking pending issues from https://github.com/e-mission/e-mission-phone/pull/1000

JGreenlee commented 1 year ago
shankari commented 1 year ago
shankari commented 1 year ago

Feedback from @mRaffill

Multimodal trip Unimodal trip
Image Image
JGreenlee commented 1 year ago

trip split is missing in the detail screen if it is unimodal; bothered me because it wasn't consistent

Is it necessary to show the same exact information twice?

I see how it's a little inconsistent visually, but I think displaying the same information twice is similarly uncomfortable. I originally wrote it that way and it was off-putting.

Maybe there's some placeholder text that should be shown to remedy the visual inconsistency?

shankari commented 1 year ago

Time use only: so not a blocker to push NREL OpenPATH to production

Activities are now getting duplicated multiple times (I added three activities, ended up with 6 (3 + 2 + 1) No place between draft trips
Image Image
shankari commented 1 year ago

Sharing QR code from the profile screen (Profile -> My OPCode -> Share button) does not do anything.

Abby-Wheelis commented 1 year ago

I'm seeing errors on my dashboard in both Android and iPhone testing. Both are popup alerts that clear when I tap ok, occur when I "refresh" the dashboard screen. After clearing the error, the all the cards on the dash have no information in them.

Android error reads: "Error loading aggregate data, averages not avaliable" "cannot read property of undefined (reading 'drove_alone') TypeError: Cannot read properties of undefined (reading 'drove_alone') an o.getMet ... at e.fillCalorieAggVals ... at e.fillAggregateVals ..."

iPhone error reads: "Error loading aggregate data, averages not avaliable" "undefined is not an object (evaluating 'i[e]') ... promiseReactionJob@[native code]

shankari commented 1 year ago

@Abby-Wheelis are you able to reproduce this in the emulator?

Abby-Wheelis commented 1 year ago

@shankari no, not yet, everything seems fine in the emulator (using a current branch of framework_migration_prep)

I do see a lot of console warnings when I refresh about illegal modes and unknown modes, but there is no error popping up in the phone and the dashboard does populate with text in the cards.

Screenshot 2023-07-13 at 4 11 33 PM

shankari commented 1 year ago

no, not yet, everything seems fine in the emulator

And this logging in with your test opcode to the staging environment?

So: on your physical phone, logging on with opcode S1 to staging, you get the error on the emulator, logging on with opcode S1 to staging, you don't get the error?

shankari commented 1 year ago

I confirmed that, on the emulator, logging in to a test stage user, I don't see any error

JGreenlee commented 1 year ago
  • also seems to have some significance like bike is good because it is green and walk is bad because it is a shade of red

"Walk" is supposed to be "brown" but I can see how it looks reddish and thus perceived as 'bad'. I have not changed the colors from what they were in the diary tab, but I do think we could revise the color coding to be more semantic. I might suggest something like this:

"Bad" highest-emission modes:

"Good" emission-free modes:

"So-so" modes:

Unprocessed or Unknown - grey

Would that be loosely accurate enough?

JGreenlee commented 1 year ago

Sometimes dates are wrong

If you take a hard look at the timestamps of this trip and place, you realize that the trip should have been marked as only July 1, while the place was an overnight stay between July 1 and July 2. The timestamps are right, but the determination of which days the trip/place fell on is wrong. This probably has to do with timezones.

Abby-Wheelis commented 1 year ago

Sharing QR code from the profile screen (Profile -> My OPCode -> Share button) does not do anything.

I just ran into this working with the profile migrations! I think it's probably something that broke when the QrCode was migrated to a React component. It seems the share code for the QR required going and getting the link to the code from the element const c = document.getElementsByClassName('qrcode-link'); const cbase64 = c[0].getAttribute('href'); which is no longer the way that element is displayed. If that link is the same as the "tokenURL" then something like this : const cbase64 = "emission://login_token?token="+settings?.auth?.opcode;might be a good replacement? I've tested in the devapp but it's a little hard to tell if things share properly there.

JGreenlee commented 1 year ago

I just ran into this working with the profile migrations! I think it's probably something that broke when the QrCode was migrated to a React component. It seems the share code for the QR required going and getting the link to the code from the element const c = document.getElementsByClassName('qrcode-link'); const cbase64 = c[0].getAttribute('href'); which is no longer the way that element is displayed. If that link is the same as the "tokenURL" then something like this : const cbase64 = "emission://login_token?token="+settings?.auth?.opcode;might be a good replacement? I've tested in the devapp but it's a little hard to tell if things share properly there.

Good catch. We should be able to use the value that's in $scope.tokenURL, which is the same as "emission://login_token?token="+settings?.auth?.opcode would be

JGreenlee commented 1 year ago

It is probably broken in intro.js as well, it's the same shareQR function

JGreenlee commented 1 year ago

(fixes in https://github.com/e-mission/e-mission-phone/pull/1005)

TODO now (P0/P1):

TODO soon (P2):

TODO later / cleanup (P3):

Need more info:

JGreenlee commented 1 year ago

I will probably address the most pressing issues this weekend.

Keep em coming!

shankari commented 1 year ago

I think this was already reported by Nitish, but when you sign up for the app for the first time (as I did with the LG), then there are no trips, and the load keeps spinning.

shankari commented 1 year ago

Feedback from @jRaffill

shankari commented 1 year ago

version 1.3.7 is not working on iPhone6, iOS 12. 100% reproducible in an iOS13.7 emulator @JGreenlee, what are we transpiling to?

The error is:

[Error] SyntaxError: Unexpected token '='. Expected an opening '(' before a method's parameter list.
    (anonymous function) (bundle.js:9593)

which seems to be

  9592 class Card extends react__WEBPACK_IMPORTED_MODULE_0__.Component {
  9593   static defaultProps = {
  9594     shadowEnabled: false,
  9595     gestureEnabled: true,
  9596     gestureVelocityImpact: GESTURE_VELOCITY_IMPACT,
  9597     overlay: _ref => {
  9598       let {
  9599         style
  9600       } = _ref;
  9601       return style ? /*#__PURE__*/react__WEBPACK_IMPORTED_MODULE_0__.createElemen       t(react_native__WEBPACK_IMPORTED_MODULE_3__["default"].View, {
  9602         pointerEvents: "none",
  9603         style: [styles.overlay, style]
  9604       }) : null;
  9605     }
  9606   };
JGreenlee commented 1 year ago

version 1.3.7 is not working on iPhone6, iOS 12. 100% reproducible in an iOS13.7 emulator @JGreenlee, what are we transpiling to?

All Typescript is being transpiled to ES5, which should be safe. Any remaining JavaScript is not being transpiled - maybe there's a problem there.

Can you show/send me error logs (from a dev build)? I am not able to test iOS<15 due to issues with my XCode installation.

shankari commented 1 year ago

Added logs to https://github.com/e-mission/e-mission-docs/issues/932#issuecomment-1637153502 This seems to be from react code? Unless line numbers in bundle.js are off somehow...

shankari commented 1 year ago

Card is actually from react-native-paper. Maybe these are related: https://github.com/callstack/react-native-paper/issues/2261 https://github.com/callstack/react-native-paper/issues/689

JGreenlee commented 1 year ago

Yes, I think that it is probably some incompatibility between react-native-paper and webpack.

I could look into it, but let me first ask this: do we actually need to support iOS 13? I believe Apple themselves consider it obsolete - all phones that could run iOS 13 can be upgraded to later versions. My understanding is that no one is stuck on iOS 13 unless they just refuse to upgrade. Should we, or do we need to, make special accommodations for situations like that? My older iPhones are using iOS 15/16 with no problems.

shankari commented 1 year ago

we don't need to support iOS 13, but we do need to support iOS 12, because iPhone 6 phones cannot be updated beyond iPhone 12. We told the current set of deployers that we support iOS 12, and I did the work #924 to actually implement it after receiving an email from a user who had an iPhone 6.

Most commercial apps can choose to drop support for older versions since they can't make a lot of money off poor people. But as a public app, we should try to support the largest variety of phones possible. On android, for example, we support API 22+, although I don't think we are testing with any android 22 phones. I have android 23 test phones, though.

shankari commented 1 year ago

Quick check on actual deployment percentages: https://developer.apple.com/support/app-store/

~ 6% of phones are running an OS below iOS 15. I think that if a version gets to 2-3%, we can consider dropping support if it is too onerous to maintain.

JGreenlee commented 1 year ago

I don't think the above links are detailing the same issue as we are having.

I found out that, surprisingly, Safari did not support static fields in class definitions until 14.x (https://caniuse.com/?search=static%20fields) This would explain why static defaultProps = ... fails.

The TypeScript transpilation we have set up only applies to our code, not our dependencies (we sort of expect React libraries to have already compiled into 'safe' JS - but in this case it's not safe enough for us)

I think we can remedy this with Babel and point it to RN Paper (+ any other affected dependencies) Might also be nice to post an issue on RN Paper with the solution, for the benefit of the open source community

JGreenlee commented 1 year ago

So, it is not too bad. It could be worse. I suppose this is to be expected when rapidly modernizing.

shankari commented 1 year ago

you could set it up so that it only transpiles the dependencies in node_modules during production. I know you were concerned about the performance of the compile/test cycle during development; and I don't think we need to develop on iOS 12. It is enough if the resulting production app works on iOS12/iPhone6

shankari commented 1 year ago

I also noticed some weirdness with the filter, not sure if others have as well. This is on on an android test phone running android 7.0 (API 24).

When I loaded the trips, the filter was in "All Trips" but it seemed like it was actually in "To Label" - it said that there were no more trips to load, although there clearly were. And then once I labeled one of those trips, there was a giant gap at the top of the screen.

All trips showing "no more trips", possibly mislabeled "To Label" Because there were actually more trips Wierd gap at the top after labeling
Image Image Image
Abby-Wheelis commented 1 year ago

I just ran into a similar situation on my Android, the "all trips" page is only showing unlabeld trips right now. After I labeled them and refreshed the page it went back to showing all of the trips. The infinite loading is also happening to me on the "to label" screen if I don't have any trips that require a label.

JGreenlee commented 1 year ago

I have a solution for the infinite loading spinner.

I will add the filter weirdness to my list of things to check out.

As for the 'weird gap', what's weird about it? What would be expected?

shankari commented 1 year ago

As for the 'weird gap', what's weird about it? What would be expected?

Since we only have one trip, that it would be at the top of the screen

JGreenlee commented 1 year ago

you could set it up so that it only transpiles the dependencies in node_modules during production. I know you were concerned about the performance of the compile/test cycle during development; and I don't think we need to develop on iOS 12. It is enough if the resulting production app works on iOS12/iPhone6

I can make the appropriate change here, but I won't have a way to verify that it fixed the issue because don't have a way to run a Simulator on iOS < 14

shankari commented 1 year ago

no worries, i can test on my iphone6

Abby-Wheelis commented 1 year ago

Make the symbols on the map and the origin/destination text be the same

With this, were we thinking change the card or change the map? I think both are possible, but in the experimenting I've done so far neither format easily.

Screenshot 2023-07-21 at 1 50 22 PM

Above is an easy change just swapping from a map-marker-star to star-circle-outline (and a star-circle is actually an even closer match). Unfortunately, the icon library doesn't have something like that for a flag.

There are ways to style them closer to what appears on the map (outline, fill) but they just look worse from what I've tried. It is also possible to embed a React element into a marker on the Leaflet map, but when I tried this with an IconButton there were a lot of warnings in the console and the icons were off-placed when I tried.

Does anyone have any opinions on what the best end-goal is here?

Abby-Wheelis commented 1 year ago

Some more questions about P2 fixes/goals for when I go to address these:

fix 'weird gap' above trip list if only 1 trip is loaded

So display the card at the top if there's only one? What about if there's two (there would still be a gap)? I know normal is for the most recent trip to be at the bottom since that’s what you see first - would it be more confusing for the location of the "most recent trip" to change?

reconsider color theming of label tab:"overall background is gray but the background for each card is blue"

I get why it's slightly blue, it falls in line with the the red and green schemes for draft and untracked. Would the concern here be addressed by a slightly more distinct shade of blue? I could also see how having all the cards be "brightly" colored could overwhelm a user. Anyone have a strong opinion of a change to be made here?

enforce min/max zoom on Leaflet maps:"in the detail screen, I zoomed out all the way and my trip disappeared and when I zoomed in again, the trip wasn't there"

I was unable to recreate this behavior (tried emulator, android, and iOS). @shankari can you give more details about the conditions that caused this? If the trips stop disappearing, do we still want to enforce zoom? It seems like zoom 1 (1/4th of the earth) would be a good min, and zoom 18 (a few blocks) is a typical max. leaflet zooms

shankari commented 1 year ago

Does anyone have any opinions on what the best end-goal is here?

From @mRaffill The flag is less of an issue. The main goal is to have it look consistent. The flag is OK because it is same color and same shape. Star is better now because it is the same shape, would be better if it was the same color.

JGreenlee commented 1 year ago

Does anyone have any opinions on what the best end-goal is here?

The best end-goal is don't use Ionicons anymore; pick 2 icons from https://pictogrammers.com/library/mdi/ and unify to those in all places they are used.

maybe 'map-marker' and 'flag'

JGreenlee commented 1 year ago

Relevant to the 'weird gap' issue: https://github.com/Shopify/flash-list/discussions/773

There is no posted solution or workaround.

shankari commented 1 year ago

There is no posted solution or workaround.

I guess we can skip that then

shankari commented 1 year ago

ok so we don't get a WSOD on the iPhone 6 (iOS 12) BUT we don't display any trips. ARGHHH!!!

While logged in as the same user:

iPhone 6 no trips Android shows trips iOS 13 emulator shows trips
iphone6_no_trips Screenshot_2023-08-11-17-46-37-92_5062a5234f99d532322a40d030e8949e simulator_screenshot_127571BF-D79E-4A60-9499-C88485C3C3B8

Maybe we should drop support for iOS 12....

shankari commented 1 year ago

This is turning out to be super hard to debug. Note that we cannot use the iOS13 emulator to test since everything works there. So I tried to install a dev version of the app on the iPhone and connect to it. The first part of this worked but the second part did not. Concretely, while trying to connect to the WKWebview on iOS12 using safari, the console would crash and all further attempts to connect would show that safari was "connecting"...

Screenshot 2023-08-11 at 6 36 37 PM
shankari commented 1 year ago

Trying to use the console to read the full phone logs, I get the attached gzip file. It seems to display javascript logs, but I don't see any errors or backtraces there.

openpath_console_logs.gz

The only errors are

error   18:13:51.375778-0700    NREL OpenPATH   nw_protocol_boringssl_get_output_frames(1301) <private>[0x143d1a5c0] get output frames failed, state 8196
error   18:13:51.377264-0700    NREL OpenPATH   nw_protocol_boringssl_get_output_frames(1301) <private>[0x143d1a5c0] get output frames failed, state 8196
...
error   18:13:51.382031-0700    NREL OpenPATH   TIC Read Status [11:0x0]: 1:57
error   18:13:51.382209-0700    NREL OpenPATH   TIC Read Status [11:0x0]: 1:57
...
default 18:14:52.608238-0700    NREL OpenPATH   error {"data":"","status":404,"config":{"method":"GET","transformRequest":[null],"transformResponse":[null],"jsonpCallbackParam":"callback","url":"json/trip_confirm_options.json","headers":{"Accept":"application/json, text/plain, */*"}},"statusText":"","xhrStatus":"complete"} while reading confirm options, reverting to defaults
default 18:14:52.615125-0700    NREL OpenPATH   error {"data":"","status":404,"config":{"method":"GET","transformRequest":[null],"transformResponse":[null],"jsonpCallbackParam":"callback","url":"json/trip_confirm_options.json","headers":{"Accept":"application/json, text/plain, */*"}},"statusText":"","xhrStatus":"complete"} while reading confirm options, reverting to defaults
...
error   18:20:07.486121-0700    NREL OpenPATH   Background task expired while holding WebKit ProcessAssertion (isMainThread? 1).
default 18:18:17.895639-0700    NREL OpenPATH   DEBUG: while reading data from nominatim, status = 200 data = {"place_id":320540621,"licence":"Data © OpenStreetMap contributors, ODbL 1.0. https://osm.org/copyright","osm_type":"way","osm_id":417094216,"lat":"37.390927285714284","lon":"-122.08640787755103","display_name":"568, XXX, Santa Clara County, CAL Fire Northern Region, California, 94041, United States","address":{"house_number":"568","road":"XXX", "county":"Santa Clara County","state_district":"CAL Fire Northern Region","state":"California","ISO3166-2-lvl4":"US-CA","postcode":"94041","country":"United States","country_code":"us"}}
JGreenlee commented 1 year ago

Since it's not WSOD but is specific to the Label screen, I have a hunch that it has to do with our use of the Map object, which is used for timelineMap. It's already being transpiled into older JS, was tested against older emulators, and shown to work there, but maybe not as old as iOS 12.

If that is the issue, I can go back on that and use a different data structure if I have to.

JGreenlee commented 1 year ago

But since Apple no longer supports tooling to emulate and debug iOS 12, I don't know a way to confirm my suspicions - short of making the change and observing whether it works.

shankari commented 1 year ago

But since Apple no longer supports tooling to emulate and debug iOS 12, I don't know a way to confirm my suspicions - short of making the change and observing whether it works.

Right. I think that this is the real problem. We don't know what the problem is and/or how to fix it.

I spent some time hacking together a script to see how widespread the problem is, and there is exactly one iPhone 12 user in a real program [1].

[1] Technically, there is also one in nrel_commute, but I am pretty sure that is me logging in with one of the test iPhones.

shankari commented 1 year ago

verified that there is only one user - relaunched the app on my personal phone and the android test phone and the nrel_commute user went away. Let's see if this one user has any recent trips.

shankari commented 1 year ago

Unfortunately, the most recent trips for the user are from today 😦 If we set the min compatible version of this version to 13, I wonder if iOS 12 users will stay with 1.3.3 In that case, we can just have 1.3.3 be the last "ios12 compatible version"