LibreShift / red-moon

Android screen filter app for night time phone use.
GNU General Public License v3.0
647 stars 81 forks source link

updated dependencies, gradles, and a bit of code #224

Closed TacoTheDank closed 5 years ago

TacoTheDank commented 5 years ago

successfully built all four different apk variants (fdroid and playstore, both debug and release) with these changes

i can provide them if you wish to test them

smichel17 commented 5 years ago

Thank you for the PR!

Unfortunately, this is really hard to review. Could you break it down into smaller commits, so I can understand the intention of each of the changes? Many of them look like they were made automatically by Android Studio, so I'm not sure if you made all of them on purpose.

Also:

Thanks again!

smichel17 commented 5 years ago

Could you break it down into smaller commits, so I can understand the intention of each of the changes?

On this point, I'm pretty sure that, by default, Android Studio stages all changes automatically, making this more difficult than it needs to be. I've always preferred to turn off the git integration in Studio, and then manually

git add $file1 file2 …
git commit -m '$commit_message'
git push
smichel17 commented 5 years ago

Finally, if you're interested in working on features or bug fixes in Red Moon (rather than just updating dependencies), I'm happy to walk you through the code base & help as needed. The easiest way to communicate about that sort of thing is probably irc/matrix/gitter (links in the readme).

TacoTheDank commented 5 years ago

lol travis is evil, i hate it so much

ill see if i can figure out how to break this down into smaller commits (well i do know, its just doing it correctly that can be challenging)

most/all of these changes i made myself, other than the rearranging imports (which it shows up as yellow to alert me to it)

also not sure what an editor config file looks like

TacoTheDank commented 5 years ago

ill group each batch of commits by the main changes made

TacoTheDank commented 5 years ago

just a git pull

smichel17 commented 5 years ago

just a git pull

From your branch, assuming that origin is this repo and not your fork,

git rebase origin master

is a way to get the most recent changes (in this case, just translations) without filling up your branch's history with merge commits.

TacoTheDank commented 5 years ago

shh shhh shhhh i was literally just about to push the changes right now xd

TacoTheDank commented 5 years ago

wat it closed hmm

TacoTheDank commented 5 years ago

ah there we go

TacoTheDank commented 5 years ago

@smichel17 all i did was git remote add upstream [base url], and then did git fetch upstream (i think for this i did), im learning how 2 code yayyyyy

smichel17 commented 5 years ago

In that case it would be

git rebase upstream master
TacoTheDank commented 5 years ago

@smichel17 shhh let me feel smart lol

TacoTheDank commented 5 years ago

either way it did what i intended, so yeah

TacoTheDank commented 5 years ago

also my origin would be my fork of the project (to make the changes), which doesnt have the upstream (base repo) changes, so i would have to pull from the base repo (or git fetch upstream after setting the remote) anyways

TacoTheDank commented 5 years ago

anyways each of the three commits correspond to their files, so gradles = git and gradle changes, xml = xml changes, and kotlin = kt changes

smichel17 commented 5 years ago

I can and will review this now, but in a couple places I'll ask, "Why did you change this?" In the future, think of commit logs as your way of explaining that ahead of time.

For example, you might do:

TacoTheDank commented 5 years ago

kk

smichel17 commented 5 years ago

Some general advice (no change needed this time): Try to make sure each of your commits compiles.

There are a few exceptions, where smaller commits are easier to understand but there's no way to make them compile. Often, though, those exceptions are a warning sign that you're trying to change too much at one time (which often leads to bugs). If you look back in Red Moon's commits and issues, you'll see I learned this the hard way ;)

smichel17 commented 5 years ago

Starting on (most of) the kotlin now.

smichel17 commented 5 years ago

@TacoTheDank Feel free to ping me in one of the chat rooms linked in the readme if you have questions that would be easier to answer in real time than in the comments here (particularly with git).

smichel17 commented 5 years ago

@TacoTheDank I've resolved all the comments which were "just FYI". Everything left, I'd like to revert (or argue for why the change is better).

In the future I should probably be explicit about which ones were requesting changes and which were just fyi the first time I review.

TacoTheDank commented 5 years ago

@smichel17 lol seems fair, i'm ok with you reverting stuff you'd like to keep (after all, it's your project)

smichel17 commented 5 years ago

I've manually incorporated a bunch of the changes from this PR. Some, I intentionally omitted; it's also possible I missed some by accident, so if there's anything I missed that you think should be included, don't let that stop you from submitting a PR.

My commits are sitting in a branch named release, which I'll merge soon (and then do a release, as the name suggests). @TacoTheDank Feel free to take a look at how I've structured my commits, there.

TacoTheDank commented 5 years ago

Thanks to you, my commits are a lot cleaner now overall in my own repositories and others I contribute to :)