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

Disabled Commenter image loading and view #4330

Closed
wants to merge 14 commits into from
Closed

Disabled Commenter image loading and view #4330

wants to merge 14 commits into from

Conversation

4D17Y4
Copy link
Contributor

@4D17Y4 4D17Y4 commented Sep 27, 2020

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • record videos
  • create clones
  • take over the world

Fixes the following issue(s)

Relies on the following changes

  • The visibility of itemThumbnailView is set to gone and the image loading attribute (imageURI) is set to null.
    To further improve view, I've added extra horizontal padding to the comment item and set it to 20dp which was 12dp initially.
    (Declared constants for the same in dimensions)

Testing apk

app-debug.zip

Agreement

@opusforlife2 opusforlife2 added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Sep 27, 2020
@Stypox
Copy link
Member

Stypox commented Sep 27, 2020

@Aditya-Srivastav thank you for your contribution!
You shouldn't just hide the commenter image completely for all users, you should add a setting instead. I still want to see commenter avatars ;-)

@4D17Y4
Copy link
Contributor Author

4D17Y4 commented Sep 27, 2020

Hey @Stypox ,
I was thinking the same.
Can u elaborate on where to add this item in setting .
Will start work as soon as it is clear.
Happy to contribute.

@TobiGr
Copy link
Contributor

TobiGr commented Sep 27, 2020

and please do not update gradle. That should be done in a separate PR :)

@4D17Y4
Copy link
Contributor Author

4D17Y4 commented Sep 27, 2020

Sure @TobiGr
Will fix it.
Thanks 👍

@Stypox
Copy link
Member

Stypox commented Sep 27, 2020

Can u elaborate on where to add this item in setting .

I was thinking about just using the already existing "Settings -> Content -> Load thumbnails". What do you think @opusforlife2 ?

@opusforlife2
Copy link
Collaborator

Makes sense.

@4D17Y4
Copy link
Contributor Author

4D17Y4 commented Sep 27, 2020

Kool
On it ;)

@opusforlife2
Copy link
Collaborator

@Aditya-Srivastav Amul Kool? :P

@4D17Y4
Copy link
Contributor Author

4D17Y4 commented Sep 28, 2020

Done!
Would love to make any changes if required.
Updating messed up the commit history sorry for that :/

SS shows the transition when Load Thumbnails is disabled.

Didn't have to set path to null, cuz it was done previously.
Don't bother about the padding and all, it was fixed.

Finally the result Screen Shot :)

@Stypox @TobiGr
Let me know if I missed something .

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you :-D

Could you upload a debug apk? You can do that with "Android Studio -> Build -> Build Bundle(s)/APK".

I just saw the branch you are making the pull request from is dev, but it is usually considered good practice to choose a different name for PR branches; NewPipe also relies on this to build an APK with a different package name so that it can be side-installed with other NewPipes. You can do this using git via command line with:

  • git branch NAME to create a new branch equal to the currently active one
  • git checkout NAME to switch to a specific branch



When the PR will be finished, I'll ask you to squash all of the commits into one. This can be done with git rebase HEAD~X, replacing X with the total number of commits in the PR; then in the editor that opens up replace every occourrence of "pick" at the beginning of a line with "squash" except for the first line; then save and exit.

@4D17Y4
Copy link
Contributor Author

4D17Y4 commented Sep 28, 2020

@Stypox

Could you upload a debug apk? You can do that with "Android Studio -> Build -> Build Bundle(s)/APK".

Done ;)

I just saw the branch you are making the pull request from is dev, but it is usually considered good practice to choose a different name for PR branches; NewPipe also relies on this to build an APK with a different package name so that it can be side-installed with other NewPipes. You can do this using git via command line with:

  • git branch NAME to create a new branch equal to the currently active one
  • git checkout NAME to switch to a specific branch

Should I close this one and start a new PR ?

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

I tested the apk and it works as expected. Thank you!

app/src/main/res/values/dimens.xml Show resolved Hide resolved
@Stypox
Copy link
Member

Stypox commented Sep 29, 2020

Should I close this one and start a new PR ?

No, there is no need this time

Stypox
Stypox previously approved these changes Sep 29, 2020
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Great! Thank you again!

@TobiGr
Copy link
Contributor

TobiGr commented Sep 29, 2020

thank you, the commits should be squashed

@Stypox
Copy link
Member

Stypox commented Sep 29, 2020

@4D17Y4 I saw you did a merge commit inside these 14 commits, and that can cause trouble with rebasing and squashing. If you need to, feel free to create another PR with the same exact changes in a single commit and then close this one ;-)

@4D17Y4
Copy link
Contributor Author

4D17Y4 commented Sep 29, 2020

Thx a lot @Stypox
That was causing errors.
Will create a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants