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.

Sign up for GitHub

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

Jump to bottom

Miscellaneous updates and cleanup #192

Closed
wants to merge 4 commits into from
Closed

Miscellaneous updates and cleanup #192

wants to merge 4 commits into from

Conversation

TacoTheDank
Copy link
Contributor

@TacoTheDank TacoTheDank commented Jan 19, 2021 •
edited

Important

Please confirm that you:

[x] read the contributing section
[x] agree to the license and the copyright

Thanks for your intention to contribute to the project!

1st commit

  • Add gradle wrapper validation action (see this)

2nd commit

  • Clean up .gitignore
  • Update minimum cmake version
  • Remove unused lifecycle-extensions library

3rd commit

  • Delete generated .idea

4th commit

  • Fix a few performance lints

M66B
M66B reviewed Jan 19, 2021
View reviewed changes
app/build.gradle Outdated Show resolved Hide resolved
@M66B
Copy link
Owner

M66B commented Jan 19, 2021

JavaMail and OpenPGP API should not be changed when there is no need because this will make synchronizing with the original source code more difficult.

M66B added a commit that referenced this pull request Jan 19, 2021
@M66B
Fixed build gradle
3179b54 
Refs #192
M66B added a commit that referenced this pull request Jan 19, 2021
@M66B
Small improvement
088967c 
Refs #192
@M66B
Copy link
Owner

M66B commented Jan 19, 2021

If I don't merge everything, this doesn't mean I don't appreciate what you are trying to do here!

@TacoTheDank
Copy link
Contributor Author

If I don't merge everything, this doesn't mean I don't appreciate what you are trying to do here!

It's completely fine! Thanks 😃

@TacoTheDank TacoTheDank requested a review from M66B January 20, 2021 19:03
M66B
M66B reviewed Jan 20, 2021
View reviewed changes
Copy link
Owner

@M66B M66B left a comment

Choose a reason for hiding this comment

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

What I wanted to say is that each time the build tools are updated or the NDK is updated, even if it is a minor version update, the gradle file needs to be changed.

app/build.gradle Outdated Show resolved Hide resolved
app/build.gradle Show resolved Hide resolved
app/src/main/java/eu/faircode/email/ActivityView.java Outdated Show resolved Hide resolved
app/src/main/java/eu/faircode/email/ActivityCompose.java Outdated Show resolved Hide resolved
app/src/main/java/eu/faircode/email/AdapterMessage.java Outdated Show resolved Hide resolved
.github/workflows/gradle-wrapper-validation.yml
@@ -0,0 +1,10 @@
name: "Validate Gradle Wrapper"
Copy link
Owner

Choose a reason for hiding this comment

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

If I correctly understands how this work, this creates an external dependency, right?

Copy link
Owner

Choose a reason for hiding this comment

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

This might not work for F-Droid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually fine, no external dependency is used. Slide uses this validation tool and still builds with F-Droid.

.gitignore Outdated
@@ -1,27 +1,16 @@
*.iml
.gradle
/local.properties
/.idea/caches
Copy link
Owner

Choose a reason for hiding this comment

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

I like to be specific about what is excluded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying you want to keep some of the .idea files?

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
app/CMakeLists.txt
@@ -1,7 +1,7 @@

# https://developer.android.com/studio/projects/configure-cmake

cmake_minimum_required(VERSION 3.4.1)
cmake_minimum_required(VERSION 3.10.2)
Copy link
Owner

Choose a reason for hiding this comment

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

Did you test this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In what manner, exactly? I built an APK and tested it, and it seemed to work perfectly fine. Is there another way it should be tested? I'm not very familiar with CMake stuff, really.

M66B added a commit that referenced this pull request Jan 25, 2021
@M66B
Updated .gitignore
3942f16 
Refs #192

https://intellij-support.jetbrains.com/hc/en-us/articles/206544839-How-to-manage-projects-under-Version-Control-Systems
M66B added a commit that referenced this pull request Jan 25, 2021
@M66B
Cleanup
43d01b3 
Refs #192
@M66B
Copy link
Owner

M66B commented Jan 25, 2021

Lifecycle extensions are definitely required (in SimpleTask.java).

@TacoTheDank
Copy link
Contributor Author

TacoTheDank commented Jan 27, 2021 •
edited

Lifecycle extensions are definitely required (in SimpleTask.java).

Fixed this. Wasn't being used by the extensions library actually, it was just transitively declaring the library that contained the class.

@TacoTheDank TacoTheDank requested a review from M66B January 27, 2021 15:38
@M66B
Copy link
Owner

M66B commented Jan 27, 2021

You have requested a review, but I believe I have reviewed everything already?

@TacoTheDank
Copy link
Contributor Author

You have requested a review, but I believe I have reviewed everything already?

I have fixed the unresolved import error (the lifecycle service). Please review the commits and make sure you like them, thanks! :) (not meaning for this to sound passive-aggressive or anything lol)

@M66B M66B force-pushed the master branch 7 times, most recently from 99b4dbb to 22a6d2a Compare February 7, 2021 20:46
@M66B M66B force-pushed the master branch 4 times, most recently from 02b92f9 to 62ae7db Compare February 12, 2021 11:41
@M66B M66B force-pushed the master branch 6 times, most recently from fc034f6 to f7713a6 Compare March 1, 2021 06:20
@M66B M66B force-pushed the master branch 5 times, most recently from 18b341d to c17f6be Compare March 2, 2021 13:33
@M66B M66B force-pushed the master branch 7 times, most recently from 77d9abd to 1b42e8c Compare March 27, 2021 19:15
@M66B M66B force-pushed the master branch 4 times, most recently from 4c85d5c to 136b4a4 Compare April 7, 2021 07:13
@M66B M66B force-pushed the master branch 6 times, most recently from 9178944 to 524aefa Compare April 14, 2021 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@M66B M66B Awaiting requested review from M66B

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@TacoTheDank @M66B