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

Feature/percentage mode #5

Merged

Conversation

athkalia
Copy link
Contributor

Added percentage mode functionality, and updated the sample project with a mini-example

Review on Reviewable

@athkalia
Copy link
Contributor Author

Hey, I am not quite familiar with Github, or gradle or travis :D So give me a shout on anything that needs updating.

Cheers

@athkalia
Copy link
Contributor Author

Travis failed, but this is cause it's trying to compile the sample project against the old version of the library (1.02) that doesn't have the percentage.

How do we handle this ?

@JorgeCastilloPrz
Copy link
Owner

Just change the gradle dependency of the sample project (in your PR) to

compile project(:library)

(remove compile 'com.github.jorgecastilloprz:fillableloaders:1.02@aar' so it does not try to get the dependency from maven central)

@athkalia
Copy link
Contributor Author

Shall I change the version under this commit as well ? from 1.02 to 1.03 ?

…illable loaders according to a percentage. Changed dependency from maven to local for the sampleApp
@JorgeCastilloPrz
Copy link
Owner

Yes, change it to 1.03, please.

…illable loaders according to a percentage. Updated library version.
@JorgeCastilloPrz
Copy link
Owner

Hi. Can you update your branch and pull the new develop changes? I have merged a master commit which had not been merged time ago. Then i will review and test your code.

Thanks in advance!

@athkalia
Copy link
Contributor Author

I think that they are already included, this branch is already up-to-date with develop and master

@danvass
Copy link

danvass commented Sep 29, 2015

@JorgeCastilloPrz Could you please accept this and push the new version to maven?

@JorgeCastilloPrz
Copy link
Owner

If you want this PR to be merged, i would need:

  • A new sample page with the new percentage feature added by XML custom attribute.
  • A real sample of a loading which a percentage being changed along the way of a "mock" download.
  • Cleaning the code for the setPercentage() method, as it is pretty dirty.
  • Updating README file properly with the new functionality.

Thanks for your help.

@athkalia
Copy link
Contributor Author

athkalia commented Oct 7, 2015

Hey, thanks for your quick review. I'll try to get this done during the coming weekend.

@JorgeCastilloPrz
Copy link
Owner

Hello. Did you work on the last changes we talked about?. Thanks in advance.

@athkalia
Copy link
Contributor Author

hey, sorry for the delay but unfortunately it's gonna get delayed more. I am about to undergo surgery so I doubt this is gonna get looked at for at least a few weeks.

Cheers

@Letme
Copy link
Contributor

Letme commented May 2, 2016

This seems like it is not moving. Can we move this a bit just to get it merged in?

@athkalia
Copy link
Contributor Author

athkalia commented May 2, 2016

Hey, I made a pull request a long time ago but unfortunately I don't have any time to spend on this. Please feel free to continue the pull request.

Letme added 4 commits May 3, 2016 09:43
AndroidStudio proposed update of gradle version to keep in sync with
latest and greatest.
SeekBar on top of the last fragment enables user to define the
percentage *on fly*. It is also a good example of static and functional
usage of the setPercentage.
Snackbar needed a try-catch as it is throwing an
```
java.lang.NullPointerException: Attempt to invoke virtual method 'void
android.widget.TextView.setText(java.lang.CharSequence)' on a null
object reference
```
while on InstantRun and I could not be bothered to fix the real reason.
Fill percentage can now also be defined inside the layout as
fl_fillPercentage. It is declared as integer, but in case float is
really needed then we can change that later on.
@Letme
Copy link
Contributor

Letme commented May 4, 2016

I have sent PR to athkalia and I hope he can merge it in so this PR gets updated with all requested features except making setPercentage pretty (which I have no idea how).

@athkalia
Copy link
Contributor Author

athkalia commented May 4, 2016

Thanks mate, I'll have a look during the weekend.

@Letme
Copy link
Contributor

Letme commented May 4, 2016

I would prefer it sooner, since Jorge will also have to take a look at it.

@athkalia
Copy link
Contributor Author

athkalia commented May 8, 2016

Can you have a look at the checkstyle issues please?

@Letme
Copy link
Contributor

Letme commented May 8, 2016

@athkalia care to add spaces in FillableLoaderPage.java between the brackets on lines 133 and 135 as that is why CI is failing?

@athkalia
Copy link
Contributor Author

athkalia commented May 8, 2016

It's passing locally for me, same for you @Letme I take it ?

@Letme
Copy link
Contributor

Letme commented May 9, 2016

Yeah it is passing locally, I checked also the library:lint (which is failing on Travis) and it is executed and passes (finds just 3 errors instead of 4). None of the Lint warnings are produced by changes in this patch (at least on my local build).
@JorgeCastilloPrz can you advise what you want us to do?

@Letme
Copy link
Contributor

Letme commented May 10, 2016

I do not have any ideas what might be wrong. Can someone help please?

@Letme
Copy link
Contributor

Letme commented May 11, 2016

One of the differences I spotted is build number of Java (95 on my computer and 76 on Travis). @athkalia can you check yours?

$ javac -J-Xmx32m -version
javac 1.7.0_95
$ java -Xmx32m -version
java version "1.7.0_95"

I could not find any reference on Travis how to fix this.

Letme and others added 2 commits May 12, 2016 10:10
Since TravisCI produces some lint errors which we cannot reproduce
locally and therefor fix, its breakage of build was removed. Since
static checker is needed, Coverity was added. Coverity Scan is free for
OpenSource projects and much better than lint anyways. It provides a
nice outside interface to check for defects and by default it has almost
no false positives.
Replace Lint with Coverity to make TravisCI happy
@JorgeCastilloPrz
Copy link
Owner

Thanks for fixing CI :). I will retake this PR this weekend and work on it to get merged. New version will be deployed to maven central pretty soon!.

Thanks for your work guys 👍

@Letme
Copy link
Contributor

Letme commented May 12, 2016

I hope you do not have to do too much. Still I have set-up the Coverity for my fork of the project and it found 2 defects which I will probably get fixed in near future. I would suggest you set up Coverity as that is much stronger static analysis tool than lint and for opensource it is free (github enables usage as service).

@JorgeCastilloPrz
Copy link
Owner

Please, if you could, it would be nice if you can fix those issues before I
review all the changes and merge your code this weekend.

Thanks in advance!
El 12 may. 2016 3:45 p. m., "Crt Mori" notifications@github.com escribió:

I hope you do not have to do too much. Still I have set-up the Coverity
for my fork of the project and it found 2 defects which I will probably get
fixed in near future. I would suggest you set up Coverity as that is much
stronger static analysis tool than lint and for opensource it is free
(github enables usage as service).


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5 (comment)

@Letme
Copy link
Contributor

Letme commented May 12, 2016 via email

@JorgeCastilloPrz
Copy link
Owner

Reviewed 2 of 6 files at r2, 1 of 1 files at r4, 11 of 12 files at r5, 1 of 1 files at r6, 3 of 3 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@JorgeCastilloPrz JorgeCastilloPrz merged commit b2e12ef into JorgeCastilloPrz:develop May 13, 2016
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