organicmaps/organicmaps-tmp

Archived

Fork 1

More intermediate stops #1141

Merged

CasparNuel merged 2 commits from more-intermediate-stops into master

2021-08-24 08:58:45 +00:00

CasparNuel commented

2021-08-20 23:54:22 +00:00

(Migrated from github.com)

Allow more intermediate stops

Currently, the application only allows for 3 intermediate stops: A, B, C.

The minimal changes to code allow for up to 100 intermediate stops (or more?).

Currently, the C stop icon is used for testing purposes. This is supposed to be replaced with a "+" icon later in development for a more streamlined user experience.

Remarks

While looking for a way to allow for more stops, I came across the following code:

android/src/com/mapswithme/maps/routing/TransitStepView.java:112


  @DrawableRes
  private static int getIntermediatePointDrawableId(int index)
  {
    switch (index)
    {
      case 0:
        return R.drawable.ic_24px_route_point_a;
      case 1:
        return R.drawable.ic_24px_route_point_b;
      case 2:
        return R.drawable.ic_24px_route_point_c;
    }
    throw new AssertionError("Unknown intermediate point index: " + index);
  }

This piece of code clearly interacts with the route points in some way. I have not yet encountered an error with my new implementation, but I was also unable to find out what this code is used for. My implementation might break something, as this getIntermediatePointDrawable() does not take into account my 4th case once it is replaced with a "+" icon.

### Allow more intermediate stops Currently, the application only allows for 3 intermediate stops: A, B, C. The minimal changes to code allow for up to 100 intermediate stops (or more?). Currently, the **C stop icon** is used for testing purposes. This is supposed to be **replaced with a "+" icon later** in development for a more streamlined user experience. ![Screenshot_20210821-014006](https://user-images.githubusercontent.com/73889285/130303153-f09bfa2b-d5bb-4b1d-ae67-34a93fea4bee.png) ### Remarks While looking for a way to allow for more stops, I came across the following code: `android/src/com/mapswithme/maps/routing/TransitStepView.java:112` ``` @DrawableRes private static int getIntermediatePointDrawableId(int index) { switch (index) { case 0: return R.drawable.ic_24px_route_point_a; case 1: return R.drawable.ic_24px_route_point_b; case 2: return R.drawable.ic_24px_route_point_c; } throw new AssertionError("Unknown intermediate point index: " + index); } ``` This piece of code clearly interacts with the route points in some way. I have not yet encountered an error with my new implementation, but I was also unable to find out what this code is used for. **My implementation might break something, as this `getIntermediatePointDrawable()` does not take into account my 4th case once it is replaced with a "+" icon**.

biodranik (Migrated from github.com) reviewed 2021-08-21 06:59:12 +00:00

routing_common/car_model_coefs.hpp

biodranik (Migrated from github.com) commented

2021-08-21 06:59:12 +00:00

These changes are accidental and should not be here.

CasparNuel (Migrated from github.com) reviewed 2021-08-21 10:44:57 +00:00

routing_common/car_model_coefs.hpp

CasparNuel (Migrated from github.com) commented

2021-08-21 10:44:57 +00:00

The changes were created by configure.sh, I'll revert them.

biodranik (Migrated from github.com) reviewed 2021-08-22 19:28:00 +00:00

map/routing_mark.cpp

biodranik (Migrated from github.com) commented

2021-08-22 19:27:59 +00:00

We use // style comments in C++. They allow to quickly temporarily comment a block of code (including // comments) with /* */

biodranik (Migrated from github.com) requested changes 2021-08-23 10:44:03 +00:00

biodranik (Migrated from github.com) left a comment

Please fix DCO and comments, and we're good to go. Even without proper icons this can be merged. Let's see how many people will complain :)

map/routing_mark.cpp

biodranik (Migrated from github.com) commented

2021-08-23 10:43:18 +00:00

Please also add
// TODO: Properly add icons for other letters/numbers after C.

Please also add // TODO: Properly add icons for other letters/numbers after C.

biodranik commented

2021-08-23 18:51:50 +00:00

(Migrated from github.com)

You need to sign your commits with your name and email. Here is the instruction: organicmaps/organicmaps#1141/checks

You need to sign your commits with your name and email. Here is the instruction: https://git.omaps.dev/organicmaps/organicmaps/pulls/1141/checks?check_run_id=3403442659

CasparNuel commented

2021-08-23 19:03:24 +00:00

(Migrated from github.com)

@biodranik

There were some other commits inbetween my commits: so I had to do an interactive rebase and change only the commits I've made myself (otherwise other peoples commits would also have my signoff). However, after doing this, I got the following DCO text:

What exactly does this mean? Can I do anything about this? I'm not sure if I did something wrong, I'm sorry if this confuses things.

@biodranik There were some other commits inbetween my commits: so I had to do an interactive rebase and change only the commits I've made myself (otherwise other peoples commits would also have my signoff). However, after doing this, I got the following DCO text: ![Screenshot from 2021-08-23 21-00-15](https://user-images.githubusercontent.com/73889285/130503028-29ba8d34-4b52-4682-9047-6ff9a6df9eaf.png) What exactly does this mean? Can I do anything about this? I'm not sure if I did something wrong, I'm sorry if this confuses things.

biodranik commented

2021-08-23 19:05:09 +00:00

(Migrated from github.com)

Try to do rebase on top of the current master first. You may also do interactive rebase with -i, and squash/fixup all your commits into one.

CasparNuel commented

2021-08-23 19:15:28 +00:00

(Migrated from github.com)

@biodranik I'm not sure I understand correctly.

I did an interactive rebase already, my commits all have my signoff now.

The error states:

Author: Alexander Borsuk, Committer: CasparNuel; Expected "Alexander Borsuk 170263+biodranik@users.noreply.github.com", but got "Alexander Borsuk me@alex.bio".

and

Author: Sean, Committer: CasparNuel; Expected "Sean connell7@purdue.ed", but got "Sean Connelly seanconn256@protonmail.com".

@biodranik I'm not sure I understand correctly. I did an interactive rebase already, my commits all have my signoff now. The error states: ``` Author: Alexander Borsuk, Committer: CasparNuel; Expected "Alexander Borsuk 170263+biodranik@users.noreply.github.com", but got "Alexander Borsuk me@alex.bio". ``` and ``` Author: Sean, Committer: CasparNuel; Expected "Sean connell7@purdue.ed", but got "Sean Connelly seanconn256@protonmail.com". ```

CasparNuel commented

2021-08-23 20:04:24 +00:00

(Migrated from github.com)

If I did anything wrong, I can use the git reflog to undo my previous rebase.

Just let me know what I should do, as I'm not sure right now.

If I did anything wrong, I can use the `git reflog` to undo my previous rebase. Just let me know what I should do, as I'm not sure right now.

biodranik commented

2021-08-23 20:27:36 +00:00

(Migrated from github.com)

Your branch should be rebased on top of the current master. Your branch should have only your signed commits (or just one commit). You accidentally did something wrong, please try to clear the commits history by a proper rebase or undo and repeat.

CasparNuel commented

2021-08-23 20:58:12 +00:00

(Migrated from github.com)

@biodranik , Sorry for the inconvenience. I'm pretty sure it is as it should be right now. Thank you for helping me :)

biodranik (Migrated from github.com) reviewed 2021-08-23 21:51:38 +00:00

map/routing_mark.cpp

biodranik (Migrated from github.com) commented

2021-08-23 21:51:37 +00:00

Almost :) Sorry for bikeshedding, but we write // style comments on the line(s) before the code, not after it:

    // TODO: Properly add icons for other letters/numbers after C.
    default: name = "route-point-c"; break;

The comment about "+" is not needed, because 1) TODO mentions it and 2) the final solution may be different than using a "+".

Almost :) Sorry for bikeshedding, but we write // style comments on the line(s) before the code, not after it: ``` // TODO: Properly add icons for other letters/numbers after C. default: name = "route-point-c"; break; ``` The comment about "+" is not needed, because 1) TODO mentions it and 2) the final solution may be different than using a "+".