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

Progress background SmartView. #474 #475

Merged
merged 1 commit into from
Nov 20, 2017
Merged

Conversation

lemonboston
Copy link
Contributor

No description provided.

@lemonboston lemonboston requested a review from dmfs November 13, 2017 19:14
@lemonboston lemonboston force-pushed the mx/474-progress-bg-smartview branch from 9d63266 to 9c1082e Compare November 13, 2017 19:15
@lemonboston
Copy link
Contributor Author

Build fails with

com.android.build.api.transform.TransformException: com.android.ide.common.process.ProcessException: java.util.concurrent.ExecutionException: com.android.dex.DexException: Multiple dex files define Lorg/dmfs/iterables/ArrayIterable

Iterators dependency insight:

org.dmfs:iterators:1.7.1 (conflict resolution)
\--- com.github.dmfs.androidcarrot:androidcarrot:5a4baa0222
     \--- compile

org.dmfs:iterators:1.5 -> 1.7.1
\--- au.com.codeka:carrot:2.4.0
     \--- compile

Plus jems:1.13 is newly imported here.

@lemonboston
Copy link
Contributor Author

So I suppose we need to:

  • exclude iterators from both codeka.carrot and dmfs.carrot
  • build, test if sharing works runtime as well
  • create tickets-PRs to update codeka.carrot and dmfs.carrot to latest jems (shall we instead replace iterators usage with JDK classes in codeka.carrot?)

Is this right?

@dmfs
Copy link
Owner

dmfs commented Nov 13, 2017

That's right. Not sure if we can switch to jems in carrot. He wasn't quite happy about importing any library at all. I think this would only be an option if we can show big improvements for the existing code as well. Otherwise it might be easier to just copy the classes we use or replace the implementation with something less "sophisticated".

@dmfs
Copy link
Owner

dmfs commented Nov 13, 2017

Implementation-wise this PR looks good. Have you tried letting TaskFieldAdapters.PERCENT_COMPLETE return an Optional instead of wrapping the result every time?

@lemonboston
Copy link
Contributor Author

I've checked that now and TaskFieldAdapters.PERCENT_COMPLETE is an IntegerFieldAdapter, so we would either need an OptionalIntegerFieldAdapter or general Optional(FieldAdapter) but as I see it's not trivial how to do it with current FieldAdapter:

public abstract class FieldAdapter<Type>
{
  public abstract Type get(ContentSet values);

  public abstract void set(ContentValues values, Type value);

  ....
}

in get() we could easily wrap in NullSafe<T> but in set() would need the actual value. In other words would the FieldAdapter have Optional<T> or <T> as type parameter? Having Optional<T> would mean adding new Present<>() for every set() usage.
So if this is something worth attending to, I think we should have a dedicated ticket.

I've excluded iterators from the carrot dependencies, the build succeeds now. I've also tested that sharing still works runtime. So this is ready for review.

@dmfs
Copy link
Owner

dmfs commented Nov 15, 2017

Ok. Leave it then. We'll probably replace it with ContentPal sooner or later so it's not worth spending too much time with it now.
I'll check out the code.

@dmfs
Copy link
Owner

dmfs commented Nov 16, 2017

Please update AndroidCarrot dependency. Looks good otherwise.

@lemonboston
Copy link
Contributor Author

I planned to rebase this on master after #478 is merged.

@lemonboston lemonboston assigned lemonboston and unassigned dmfs Nov 16, 2017
@dmfs
Copy link
Owner

dmfs commented Nov 17, 2017

@lemonboston please rebase.

@lemonboston lemonboston force-pushed the mx/474-progress-bg-smartview branch from 0c7a87e to adb7429 Compare November 17, 2017 16:18
@lemonboston lemonboston force-pushed the mx/474-progress-bg-smartview branch from adb7429 to f8faa1d Compare November 17, 2017 16:21
@lemonboston
Copy link
Contributor Author

Squash-rebased.

@dmfs
Copy link
Owner

dmfs commented Nov 20, 2017

Despite the CI issue it works for me. So I'm going to merge this.

@dmfs dmfs merged commit aaddf5c into master Nov 20, 2017
@dmfs dmfs deleted the mx/474-progress-bg-smartview branch November 20, 2017 10:02
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.

2 participants