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.

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

Order regions by continents #395

Merged
merged 9 commits into from
Mar 7, 2018
Merged

Order regions by continents #395

merged 9 commits into from
Mar 7, 2018

Conversation

ialokim
Copy link
Collaborator

@ialokim ialokim commented Dec 12, 2017

This PR adds the ordering of transport network regions into continents and therefore an easier way of picking the requested region as suggested here by @grote.

It is already working on my personal phone, here some screenshots:
screenshots

Before merging it in, I think we should discuss the following points:

  • Should the continents present little icons at the left, something like this (see also their Terms of Use? Should they be in SVG or PNG format?
  • As almost all Regions represent countries, shouldn't we rename them in code to Country?
  • The continent list item design is, as for now, almost identical to (because copied of) the region's design. Any ideas on how to improve the distinction are very welcome!
  • Should we use the Wikipedia definition where Central America is part of North America or should we separate it as another subcontinent of America as defined in Ibero-America?
  • I think it would be necessary to translate the continent names in the respective strings.xml

@grote
Copy link
Owner

grote commented Dec 12, 2017

Thanks a lot for working on this!

Should the continents present little icons at the left, something like this (see also their Terms of Use?

I think little recognizable icons would be nice. They should be under a free license.
You should be able to re-use the RegionViewHolder if you add icons, right?

Should they be in SVG or PNG format?

In Android XML vector drawable format. SVG can usually be converted.

As almost all Regions represent countries, shouldn't we rename them in code to Country?

Which ones do not represent countries?

I personally think, we only need a region/country if there's more than a few providers in it. Otherwise, we can add the providers directly at the continent level, especially when they are supposed to cover an entire country anyway.

Should we use the Wikipedia definition where Central America is part of North America or should we separate it as another subcontinent of America as defined in Ibero-America?

To me it feels odd that Costa Rica is part of North America. Maybe use Middle America instead?

@ialokim
Copy link
Collaborator Author

ialokim commented Dec 12, 2017

I think little recognizable icons would be nice. They should be under a free license.

If I understand it right, in the proposed case there's no official licence, but 'free to use (...) for use in both personal and commercial projects' as stated here. Btw, it features an 'Central American' icon which includes Mexico...

You should be able to re-use the RegionViewHolder if you add icons, right?

Yes, but maybe we should rename it to avoid confusing?

Which ones do not represent countries?

Only Europe (with cross-border trains) and Africa (known concern) for now.

I personally think, we only need a region/country if there's more than a few providers in it. Otherwise, we can add the providers directly at the continent level, especially when they are supposed to cover an entire country anyway.

I understand your point, but that would be a little more complicated because the FastAdapter library with their expandable extention seems to support only one type of children (in this case would be or RegionItem or TransportNetworkItem). We should have an abstract parent class, not sure if this would work. But I'll look into it.

To me it feels odd that Costa Rica is part of North America. Maybe use Middle America instead?

Personally, I don't find it very intuitive because I would never search Colombia or Venezuela in Middle America as stated on Wikipedia. Why not keeping Central America for Mexiko down to Panama? (Also see icon response).

@grote
Copy link
Owner

grote commented Dec 12, 2017

[Icons]

Looking at their Terms of Use, I am quite sure they would be classified as non-free artwork by F-Droid.

Did you check https://materialdesignicons.com/ and https://emojipedia.org/ for alternative icons?

You should be able to re-use the RegionViewHolder if you add icons, right?

Yes, but maybe we should rename it to avoid confusing?

Yes, when there's a consensus on how things will be, we can rename stuff to better reflect its usage.

Only Europe (with cross-border trains) and Africa (known concern) for now.

OK, but these are continents now. We don't need those as Regions/Countries anymore.

Why not keeping Central America for Mexiko down to Panama?

That's fine with me.

@ialokim
Copy link
Collaborator Author

ialokim commented Dec 12, 2017

Did you check https://materialdesignicons.com/ and https://emojipedia.org/ for alternative icons?

I've checked them just now, they only have globes with Europe and Africa, Asia and Australia or North and South America together.

What about Creative Commons Attribution-Share Alike 4.0 International, 3.0 Unported, 2.5 Generic, 2.0 Generic and 1.0 Generic license as in this file of wikipedia. Seems like there could be an SVG for each continent we would like to include.

OK, but these are continents now. We don't need those as Regions/Countries anymore.

That's right, if it's possible to have two different child element types.

@grote
Copy link
Owner

grote commented Dec 12, 2017

CC-SA is fine. The contour lines probably would need to be simplified a bit before converted into a vector drawable, but this should work.

@ialokim
Copy link
Collaborator Author

ialokim commented Dec 20, 2017

Working on it, I found a bug in the FastAdapter library, waiting for the merge to continue working on this feature.

@ialokim
Copy link
Collaborator Author

ialokim commented Dec 21, 2017

I've already proceeded to include the continents contour lines (I've generated them myself from this Wikimedia file, published as "gemeinfrei") and changed my commit as requested to allow national transport networks to be shown as child elements of continents.

I've too worked a bit on the list (item) design to get a more ordered look and feel. See these screenshots of the current version:
out

As mentioned, there's still the error because of the bug in the FastAdapter library, but I thought of pushing it already to be able to discuss the changes.

@ialokim
Copy link
Collaborator Author

ialokim commented Dec 24, 2017

The bugfix is now merged, I've updated the libraries.

@grote
Copy link
Owner

grote commented Dec 24, 2017

Awesome! I'll try to find time for a review and test soon, but no promises.

@ialokim
Copy link
Collaborator Author

ialokim commented Dec 24, 2017

Awesome! I'll try to find time for a review and test soon, but no promises.

That would be great, but no need to hurry. Make sure you'll have a great talk at 34C3! Greetings by Nico, too.

Copy link
Owner

@grote grote left a comment

Choose a reason for hiding this comment

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

Hey, thanks again for your work! :)

I finally found some time for a first review pass and made some comments above. I am not yet sure the complicated Item/ViewHolder structure is necessary, but I will come back to this in a second pass.

@@ -113,8 +113,8 @@ dependencies {
implementation 'com.google.android:flexbox:0.3.1'
implementation 'com.mikepenz:materialdrawer:6.0.0'
implementation 'com.mikepenz:aboutlibraries:6.0.0'
implementation 'com.mikepenz:fastadapter-commons:3.0.3@aar'
implementation 'com.mikepenz:fastadapter-extensions-expandable:3.0.3@aar'
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind upgrading this to the latest 3.2.0? The CI fails for 3.0.5 for some reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, no problem. 😄 It's already at 3.2.2 🙊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*3.2.3 😆

@@ -74,13 +74,13 @@ class PickTransportNetworkActivityTest : ScreengrabTest() {
makeScreenshot("1_FirstStart")

// hack to find region position in list
val regionList = ArrayList(EnumSet.allOf(Region::class.java))
val regionList = ArrayList(EnumSet.allOf(Country::class.java)) //TODO: pretty sure it will break :/
Copy link
Owner

Choose a reason for hiding this comment

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

If you are pretty sure this will break, is this PR then even ready for review or did you just forget to remove this TODO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll look into Android tests, I've never worked with them before. But if you could give me a hint on how this test was written, would be great!

Copy link
Owner

Choose a reason for hiding this comment

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

This is an instrumentation test running on a real device, simulating user input. It is primarily used to take nice screenshots automatically. It is not run by the CI.

So you need to attach an Android device via ADB and run All tests in Android Studio.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tests are now running, thanks to the continent ordering even simplier :)

@@ -56,9 +62,9 @@ public int getLayoutRes() {
}

@Override
public void bindView(TransportNetworkViewHolder ui, List<Object> payloads) {
public void bindView(RegionViewHolder ui, List<Object> payloads) {
Copy link
Owner

Choose a reason for hiding this comment

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

This looks fishy. Why would this be called with a RegionViewHolder when there's a TransportNetworkViewHolder as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TransportNetworkItem extends RegionItem which itself extends AbstractExpandableItem setting at this point the ViewHolder class. To be able to override functions correctly, it has to be RegionViewHolder at this point.

But I'll look into it, I think I could use some anonymous typing here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've now introduced type parameters, so this problem should be fixed.

android:translateX="-82.992"
android:translateY="-45.607">
<path
android:pathData="M272.998 56.477085c-30.6305 1.42763 -61.7999 0.84337 -88.5736 17.89075 -16.4448 -5.66816 -27.9221 -1.95183 -35.0817 11.35931 -22.8063 10.88936 -14.5516 46.829195 -42.5078 52.943885 -14.450803 20.78118 -41.109203 46.53139 -28.701803 72.83371 1.4457 26.00138 -21.219 44.141 -1.9809 66.06526 15.1073 -0.23363 20.5331 32.35991 35.384003 45.50076 18.527 26.51205 47.1616 15.23162 72.4647 15.09745 22.1523 -4.82105 51.4612 -29.33518 61.7782 4.16413 25.0032 -14.04853 38.034 30.30727 20.0218 42.5551 11.0621 24.28808 31.3179 44.85661 34.1992 72.87249 10.9604 31.5683 -25.8717 58.73837 -7.2399 89.90658 20.0531 24.85866 9.1412 58.79265 28.9161 83.44667 13.5753 20.8407 4.3137 63.8253 43.092 46.2429 29.1991 3.245 54.2772 -16.293 68.2268 -40.7545 18.6511 -12.2919 3.3475 -43.0946 29.7124 -46.39433 8.0339 -20.57169 -12.316 -45.73156 17.2474 -56.0187 31.02 -8.06212 31.393 -43.02275 24.0289 -66.5604 -15.0496 -28.1345 -2.8916 -56.20902 15.996 -79.19872 16.5758 -23.45028 44.3438 -37.52646 54.61 -65.9697 10.0995 -16.43731 25.7015 -58.62637 -8.5112 -39.88071 -16.3269 2.67892 -38.5161 14.75198 -38.1844 -12.62644 -23.5852 -16.37171 -36.5365 -39.04417 -47.7096 -66.38673 0.1428 -26.23491 -36.6894 -48.74928 -21.3932 -72.90397 3.877 -40.002355 -40.1416 -18.13883 -61.7582 -24.12179 -17.539 -9.119225 -51.0148 -23.355735 -53.1682 6.48243 -24.0422 -1.92841 -40.2168 -24.328415 -65.9839 -24.975475 -1.2329 -12.86073 11.9661 -28.39646 -4.8831 -31.56996zm157.5462 48.204155l0.1253 0.0701 -0.1253 -0.0701zm-326.6843 35.40422l-0.087 0 0.087 0zm79.014 -65.033685l-0.3014 -0.34606 0.3014 0.34606zm98.4975 11.19066c-4.2808 2.32982 3.9892 1.62441 0 0zm152.1214 18.978075c1.0481 1.21896 -1.9782 -0.009 0 0zm-1.6262 0.99996c2.8413 0.74684 -2.3867 0.96986 0 0zm10.0293 4.78963c9.0849 1.4785 -6.2276 5.74926 0 0zm-339.0023 82.82072l-0.1528 0.44158 0.1528 -0.44158zm-27.858803 0.97232l4.5868 0.00075 -4.5868 -0.00075zm406.014703 9.09519l0 0.0764 0 -0.0764zm-403.817303 3.72599c-1.9972 0.97538 0.6671 4.9408 0 0zM502.0933 242.07314c-2.5274 3.55554 7.4291 3.48667 0 0zM71.869297 265.02771c3.6619 1.96241 -3.5369 4.1033 0 0zm8.0697 5.81295c1.1274 1.05532 -1.4259 0.87525 0 0zm-4.5666 5.48812c-0.5189 1.88818 1.8772 -0.48223 0 0zm2.1591 0.28873c-2.5715 -1.33194 0.2825 2.5294 0 0zm14.1247 1.92775c5.548 -0.45224 -9.1359 1.78631 0 0zm-16.5343 1.02544c-2.4293 0.1396 0.9053 1.42442 0 0zm2.3395 0.21442c-1.405 2.06978 2.4807 -0.36017 0 0zm9.8681 0.33545l-1.1673 1.86982 1.1673 -1.86982zm-7.2078 1.5371l-0.7899 1.225 0.7899 -1.225zm2.4649 0.6539c-1.5819 0.13192 1.7642 1.50076 0 0zm-6.2525 0.67514c-1.9752 1.53127 2.8772 0.79053 0 0zm8.6069 1.67297c-1.6561 2.88052 2.7424 0.13665 0 0zm26.582903 8.09949l-0.064 0.0248 0.064 -0.0248zm0.3397 0.0361l0.5053 0.0743 -0.5053 -0.0743zm-4.2907 0.80464l-0.327 0.0955 0.327 -0.0955zm-2.8046 0.30148l-0.068 0.0212 0.068 -0.0212zm10.3393 0.80251l1.5327 2.55402 -1.5327 -2.55402zm-16.576903 6.32886l-0.1847 0.017 0.1847 -0.017zm-0.397 0.0679l-0.091 0.034 0.091 -0.034zm20.464303 0.66876l0 0.16984 0 -0.16984zm0.3757 2.45214l-0.04 0.21018 0.04 -0.21018zm-16.9208 9.26293c-5.061603 0.63231 5.4912 1.8662 0 0zm101.2574 14.29671c0.1097 3.37264 -2.7619 -0.0308 0 0zm58.2803 9.00179c1.5527 0.23823 -0.8769 1.87928 0 0zm-7.5284 3.53065c-2.8482 1.65599 1.877 1.38962 0 0zm2.1591 0.36092c-3.3931 -0.0109 -0.368 0.99503 0 0zm16.9357 5.63037l0.051 0.45009 -0.051 -0.45009zm-6.8384 0.60082c-7.9871 2.98302 2.3813 7.46043 0 0zm-10.9869 17.81888c-1.7223 0.88759 1.6685 2.97618 0 0zm-4.1102 0.72396c-23.1739 16.34225 22.7988 14.63751 0 0zm24.9905 5.04015l3.7832 0.0729 -3.7832 -0.0729zm-0.9681 0.19533l-0.6029 0.10402 0.6029 -0.10402zm177.9213 8.58567c18.6922 18.67151 -24.3355 35.85554 -9.1152 5.51665 1.1747 -4.75713 6.3059 -3.11648 9.1152 -5.51665zm-185.8531 5.54756l0.1972 1.26406 -0.1972 -1.26406zm156.1301 34.83948c5.7547 15.57385 11.707 36.03641 0.8642 11.26211l-0.2652 -5.69258 -0.599 -5.56953zm78.2286 2.3099c-1.6959 4.19481 3.7885 5.32452 0 0zm-2.8088 7.1993c-3.4709 6.37251 5.5416 6.19339 0 0zm3.8194 16.99088c-3.5523 2.63503 -0.065 3.50123 0 0zm-68.6599 2.64746c4.9419 4.37319 -1.9493 3.98343 0 0zm24.3856 13.10142c12.5353 9.34666 0.5533 45.43428 2.8507 14.60687 0.7082 -4.97042 0.5552 -10.54564 -2.8507 -14.60687zm44.8752 10.70237l-0.482 0.30572 0.482 -0.30572zm-3.9745 2.43303c-2.2037 1.31735 -1.6964 1.01412 0 0zm32.7228 1.63052c-22.6121 13.30958 22.0257 16.76755 0 0zm-43.508 0.19744l0.2221 0.0902 -0.2221 -0.0902zm3.8003 0.95325l-0.1 0.0446 0.1 -0.0446zm-5.711 0.57111l-0.2335 0.29935 0.2335 -0.29935zm-15.4962 0.27387l-0.6878 0.35668 0.6878 -0.35668zm-4.6347 1.13797l0.8917 0.0297 -0.8917 -0.0297zm14.4199 0.41399l0.3396 0.30147 -0.3396 -0.30147zm3.4541 0.34769c-2.8605 0.69679 -1.7203 0.41906 0 0zm-9.5728 0.20382l1.484 0.16984 -1.484 -0.16984zm98.0325 2.32688c-5.4679 22.61953 -24.3092 31.25113 -40.2439 41.82703 8.4251 25.76743 -24.6727 57.47949 4.3408 75.82891 28.3554 -7.87684 23.0665 -45.38599 35.0182 -66.98526 4.9318 -19.09082 13.7802 -31.7672 0.8849 -50.67068zm-7.7131 10.56648c-2.1864 1.73758 3.4233 2.55496 0 0zm11.8106 30.17086c-2.2079 1.70654 -0.4682 4.86705 0 0zm-103.6331 18.31355c-1.558 -0.53209 0.8265 1.96713 0 0zm-7.297 23.61696c-0.4843 3.0056 -0.3419 2.12177 0 0z"
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please simplify this massive path, remove the translation group around it and use the proper heigth/width values already here? This is best done in Inkscape before creating the XML.

Copy link
Owner

Choose a reason for hiding this comment

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

Please do the same for the other new drawables.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you please simplify this massive path

How much more do I need to simplify? I've already simplified all them down to ~130 points.

remove the translation group around it and use the proper heigth/width values already here

Could you please give me a hint on how to do this with Inkscape? 🤔

Copy link
Owner

Choose a reason for hiding this comment

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

In Inkscape, just select the group and ungroup it. in the status bar, it will tell you whether you selected a group or a path. Just ungroup the groups until they are gone.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, the problem was the viewbox, I've removed it and simplified (a lot) the svg graphics. But I wasn't able to simplify more the actual path, within Inkscape it only lost the form but didn't reduce the number of points.

@@ -11,8 +11,8 @@

<ImageView
android:id="@+id/logo"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_width="64dp"
Copy link
Owner

Choose a reason for hiding this comment

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

This won't look good for PNG images that we still have a few of. Better leave it as it was before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure anymore, but I think this was producing different sizes which looks a bit odd. I added the value to keep a consistent text position throughout continent, country and transport network items.

How many PNG files are there? If there aren't too much, I'm ok with reverting this change here and open another PR converting the PNGs to vectorgraphics. Or at least resizing the PNGs to exactly 64 pixels width.

Copy link
Owner

Choose a reason for hiding this comment

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

Note that dp is not pixels. I wouldn't put work in enlarging PNGs. There is #238 and having the PNGs in a different size is a nice reminder of that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I've reverted this one.

@cla-bot
Copy link

cla-bot bot commented Feb 7, 2018

Thank you for your pull request and welcome to our community! We require contributors to sign our Contributor License Agreement, and we don't seem to have the user @ialokim on file. In order for your code to get reviewed and merged, please explicitly state that you accept the agreement.

@ialokim
Copy link
Collaborator Author

ialokim commented Feb 7, 2018

Hereby I explicitly state that I accept the Contributor License Agreement whenever contributing to Transportr.

@ialokim
Copy link
Collaborator Author

ialokim commented Feb 7, 2018

I've now added some commits addressing your issues. There's still pending to add more translations for the continents, or will this be added by separate PRs in the future?

grote added a commit that referenced this pull request Feb 16, 2018
@grote
Copy link
Owner

grote commented Feb 16, 2018

@ialokim thanks for the fixes! Before I look into them, would you mind looking into the lint errors that prevent the CI from succeeding?

@grote
Copy link
Owner

grote commented Feb 16, 2018

@cla-bot[bot] check

@cla-bot
Copy link

cla-bot bot commented Feb 16, 2018

The cla-bot has been summoned, and re-checked this pull request!

@cla-bot cla-bot bot added the cla-signed ✔️ The Contributor Licence Agreement was signed by all contributors. label Feb 16, 2018
@ialokim
Copy link
Collaborator Author

ialokim commented Feb 16, 2018

Before I look into them, would you mind looking into the lint errors that prevent the CI from succeeding?

Yes, there only were some unused translations that I removed now.

Copy link
Owner

@grote grote left a comment

Choose a reason for hiding this comment

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

I gave this another reading. Thanks for the improvements done so far! :)

val regionList = ArrayList(EnumSet.allOf(Region::class.java))
val context = InstrumentationRegistry.getTargetContext()
Collections.sort(regionList) { r1, r2 -> context.getString(r1.getName()).compareTo(context.getString(r2.getName())) }

Copy link
Owner

Choose a reason for hiding this comment

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

This test is run in different locales and there's some where Germany wasn't visible, so we needed to scroll to it before it could be clicked. If Europe is at the bottom of some very small screen and therefore not visible (or not visible within the Europe category) scrolling code might still be needed. I'll need to test this with all available locales before merging...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could re-include the scrolling code if you like, searching for and opening Europe, then Germany and then Deutsche Bahn?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I think that would be the safest thing to do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm, while working on it, I thought it would be much easier to reimplement the scrolling function after changing the iterations method (probably as suggested in another PR?). For now, it would be necessary to copy the code from PickTransportNetworkActivity which I think is not the best option.

Btw, I've searched for this espresso testing functions and found this documentation which states:

Performs a ViewAction on a view matched by viewHolderMatcher.

  1. Scroll Recycler View to the view matched by itemViewMatcher
  2. Perform an action on the matched view

So I don't quite understand why it doesn't work, I've tested it with another network which indeed was out of the display and it crashes. Would it be okay to handle this scrolling thing in the same PR suggested here?

Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer not to merge anything which breaks tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed the test while introducing a subRegions list for ParentRegion (so, for Continent and Country). It's now using the same hack as before.


@Override
protected String getName(Context context) {
return this.continent.getName(context);
Copy link
Owner

Choose a reason for hiding this comment

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

No need for this here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it.


@Override
protected String getName(Context context) {
return this.country.getName(context);
Copy link
Owner

Choose a reason for hiding this comment

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

No need for this here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed it. Same for TransportNetworkItem.

} else if (region instanceof Continent) {
continent = (Continent)region;
}
pos = adapter.getPosition(new ContinentItem(continent));
Copy link
Owner

Choose a reason for hiding this comment

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

You can declare pos here. No need to do it up there (and initialize it) already when it is not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, I've changed it.

android:viewportWidth="538.6851"
android:viewportHeight="645.0074"
android:width="538.68512dp"
android:height="645.00739dp">
Copy link
Owner

Choose a reason for hiding this comment

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

These should be 64dp if I remember correctly. Bonus points for using 64px in inkscape already and centering the drawing in it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It wasn't necessary because I set the width in the list item definition. But I've changed it, what did I win with the bonus points? 😆

android:viewportWidth="845.6545"
android:viewportHeight="565.9996"
android:width="845.65448dp"
android:height="565.99963dp">
Copy link
Owner

Choose a reason for hiding this comment

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

These should be 64dp if I remember correctly. Bonus points for using 64px in inkscape already and centering the drawing in it.

android:viewportWidth="458.9524"
android:viewportHeight="454.8092"
android:width="458.95242dp"
android:height="454.80923dp">
Copy link
Owner

Choose a reason for hiding this comment

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

These should be 64dp if I remember correctly. Bonus points for using 64px in inkscape already and centering the drawing in it.

android:viewportWidth="378.4156"
android:viewportHeight="610.44"
android:width="378.41562dp"
android:height="610.44dp">
Copy link
Owner

Choose a reason for hiding this comment

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

These should be 64dp if I remember correctly. Bonus points for using 64px in inkscape already and centering the drawing in it.

CountryItem countryItem = new CountryItem(country);
List<Region> networks = regions.get(country);
List<TransportNetworkItem> networkItems = new ArrayList<>(networks.size());
for (Region network : networks) {
Copy link
Owner

Choose a reason for hiding this comment

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

This triple-nested loop looks a bit crazy. I wonder if we can't simply things a bit so it is less complicated and easier to understand.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe when the networks are defined, we could already define the hierarchy (best in Kotlin), so we save ourselves these iterations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe when the networks are defined, we could already define the hierarchy (best in Kotlin), so we save ourselves these iterations?

This could simplify it a lot, indeed. Where would be the right place for it then?

Copy link
Owner

Choose a reason for hiding this comment

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

We would probably do this right in the TransportNetworks interface. But maybe as a second step after this PR is done.

@@ -146,19 +193,53 @@ private void selectItem() {
}
}
}

private HashMap<Region, List<Region>> getRegionsByRegion() {
Copy link
Owner

Choose a reason for hiding this comment

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

getRegionsByRegion() sounds a bit confusing. Maybe we can find a clearer name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could be getSubRegionsByRegion? But the functions wouldn't even be necessary when changing this whole iterations thing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this function in the latest commit.

@grote
Copy link
Owner

grote commented Mar 1, 2018

@ialokim this has conflicts now and is still missing a fix for the tests.

@ialokim
Copy link
Collaborator Author

ialokim commented Mar 6, 2018

The tests are now working again with the same hack as before and the conflicts because of the newly merged Nicaraguan provider are now also fixed.

@grote grote removed the cla-signed label Mar 6, 2018
@grote grote merged commit 70f98bf into grote:master Mar 7, 2018
@grote
Copy link
Owner

grote commented Mar 7, 2018

Thanks @ialokim, I now merged this!

@grote
Copy link
Owner

grote commented Mar 12, 2018

@ialokim I merged the improvements we talked about into master. Please have a look if everything is alright.

There still seems to be a bug with restoring the expansion and selection state when rotating the screen (best tested after clearing app data), but this seems to be an upstream bug.

@ialokim
Copy link
Collaborator Author

ialokim commented Apr 4, 2018

@ialokim I merged the improvements we talked about into master. Please have a look if everything is alright.

Are you refering to this commit? I'm not very familiar with the Kotlin language, but looks all right for me. And indeed, much more simple! 👍

Great you're already moving towards RC for the 2.0 release! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed ✔️ The Contributor Licence Agreement was signed by all contributors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants