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

fixes NPE: sets default color to grey instead of null if not defined in the configuration #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmaulat
Copy link

@dmaulat dmaulat commented Dec 18, 2016

Fix to #107, with the help of @corentin38
NPE seemed to occur also on France South-West and South-East regions too.

@patkan
Copy link
Contributor

patkan commented Jan 11, 2017

I have tested this PR by pulling in the changes locally into pte in Transportr and can confirm that it fixes #107.

@schildbach
Copy link
Owner

Ah sorry I should have commented this earlier. I'm not convinced that defaults should be defined in PTE. Imho, if the interface doesn't have a color it should say so (by using the value 'null') and the app should decide on a default color.

What do you think?

@grote
Copy link
Contributor

grote commented Jan 11, 2017

I wouldn't mind this being set by PTE, maybe also on a per-provider basis as the default color probably varies from provider to provider.

The problem with letting the individual providers take care of it is that they don't handle the @Nullable color properly and cause NPEs. We had that in the past a lot and it is still happening.

Maybe a protected getDefaultColor() method that providers can override if they like?

@schildbach
Copy link
Owner

Yes, you generally have to handle null values. There are lots of values that can be null in the PTE results. Maybe we need to add more @Nullable annotations?

@grote
Copy link
Contributor

grote commented Jan 15, 2017

I would be great if all return values that can be null would be annotated @Nullable, same for parameters.

But I see this discussion more about where/how/if default colors should be handled.

if getLineStyle() would not be overwritten in the FranceNorthEastProvider (and other Navitia providers), it would use a default color and shape for lines, right? So that is already handled/set somewhere (I haven't checked where).

We could make this more modular and use as an automatic fallback in other places when the color is null or just give the app total responsibility.

When viewing PTE as a versatile and easy-to-use library, it would be a nice addition to provide means to set a default color that is automatically used if no color is there, so this logic does not need to be re-implemented by every single program using PTE.

@schildbach
Copy link
Owner

I'll try to go through the API and add @Nullable where appropriate. I just noticed that colors are ints, and in fact the value 0 is used if there is no color. This is congruent with the Android API, but of course PTE is not only used by Android developers. We could switch all colors to Integer, but I'm not sure if it's worth it.

There are some style defaults, but only for products that have a style default in the real world. For example, in Germany S-Bahn has this dark green rounded style. In these cases, defaults are ok. But I think PTE generally – not only for styles – should not invent values. Imho the handling of missing data is up to the app using PTE.

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.

4 participants