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

Animated Profile Avatar #69

Merged
merged 21 commits into from Apr 1, 2022
Merged

Animated Profile Avatar #69

merged 21 commits into from Apr 1, 2022

Conversation

melon095
Copy link

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Added option in Misc to show SevenTV animated avatar rather than the default Twitch.
Avatars are cached. (This can be improved)

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: matt <30363562+mmattbtw@users.noreply.github.com>
@anqueue
Copy link

anqueue commented Jan 22, 2022

Functionality works as intended, but a couple things:

  • Loading the profile picture from a usercard takes longer now. Maybe load the twitch pfp first then 7tv animated pfp?
  • Show 7TV Animated Profile Picture should be on by default

@melon095
Copy link
Author

Hello, i have tried to modify the code to better fit your requirements. Please let me know if there is anything else you want me to change.

@TroyKomodo
Copy link

Hi @mmattbtw Can you do a rebase on this pr?

@mmattbtw
Copy link

Not my PR lol :^)
pinging @JoachimFlottorp

@TroyKomodo
Copy link

Whoops.

@melon095
Copy link
Author

Hope that was the correct button. 👍

@TroyKomodo
Copy link

@MrAuro @mmattbtw can you please confirm these changes work.

@anqueue
Copy link

anqueue commented Mar 25, 2022

Works as intended for me on Windows 👍

@TroyKomodo
Copy link

Almost works,

bug.mp4

Notice how the transparent layers are not respected on the image.

@TroyKomodo
Copy link

@MrAuro can u confirm this is the case. check sodapoppins user card.

@melon095
Copy link
Author

This seems to have been an error only on release which is why i did not see it. I seem to have fixed it and can verify that this fix works in the release build

2022-03-25.21-41-59.mp4

@anqueue
Copy link

anqueue commented Mar 25, 2022

@TroyKomodo Can you run actions on the latest commit, ill test once those finish

@TroyKomodo
Copy link

TroyKomodo commented Mar 25, 2022

@JoachimFlottorp still seems off on mine, but could be because i am running it from WSL and into windows.

@TroyKomodo
Copy link

@JoachimFlottorp can you verify it works on linux?

@melon095
Copy link
Author

I'll check it on a linux machine once it builds.

@anqueue
Copy link

anqueue commented Mar 25, 2022

Transparency works on latest build. (windows, qmake) works

@melon095
Copy link
Author

melon095 commented Mar 25, 2022

I have checked on linux using Virtualbox and a physical device, i have verified that the AppImage from github actions produces the same error. But building chatterino myself on linux does not show this error.

AppImage in virtualbox
https://user-images.githubusercontent.com/24413121/160210396-d52c6a64-3958-409f-9df8-6ae253b4ceaa.mp4

AppImage on physical linux
https://user-images.githubusercontent.com/24413121/160207598-d7c0ec58-eacc-48ba-82d2-140feaa635c4.mp4

Built using QTCreator in virtualbox
https://user-images.githubusercontent.com/24413121/160210455-447df0fe-c27c-4554-b93f-c8f16322d5b2.mp4

@TroyKomodo
Copy link

So is it a dependency issue then?

@melon095
Copy link
Author

melon095 commented Mar 26, 2022

I might have tracked it down to a wayland issue. I downloaded the three dependencies libwayland-dev libwayland-egl1-mesa libwayland-server0, and ran a AppImage that i built locally. Yes it did remove the transparent flickering. I also tested it on a x11 machine, and found no flickering.

https://wiki.qt.io/QtWayland#Create_and_Run_your_own_Wayland_Compositor_with_Qt5

I will modify the Ubuntu CI to install the three dependencies just to verify this fixes that. If that is alright with you.

eShrug

Someone would also need to verify on MacOS if there is any issue with this.

X11
https://user-images.githubusercontent.com/24413121/160217087-49d2397b-aa04-4dca-8326-d44bf4bfd2af.mp4

@TroyKomodo
Copy link

Thank you, yes please do update the CI in this pr.

@melon095
Copy link
Author

Hi, been busy these lately days, But i was wrong about it being wayland, good job on my side image Either way. If im correct it's only an issue on Qt 5.12.10. And Qt 5.15.2 Doesn't have it. No idea how you want to fix this, i would just remove 5.12.10 from being built. But not like i have control over upstream.

@TroyKomodo
Copy link

TroyKomodo commented Mar 29, 2022 via email

@TroyKomodo
Copy link

TroyKomodo commented Mar 29, 2022

@JoachimFlottorp Can you also update the install guide for installing the correct version of qt, and any other changes required for any dep change, such as cmake or anywhere where the minimum version has now changed.

@melon095
Copy link
Author

melon095 commented Mar 29, 2022

A few notes. VVVV

Problem at .docker/Dockerfile.build Seems like Offline Installations have been removed since Qt 5.15.

PPA Repositories travis Use has removed the meta package in version Qt 5.15 Here.
Another package which could replace is qtbase5-dev however, has different paths.

Build files such as .CI/CreateAppImage.sh requires also a fix, to fit newer paths.

@TroyKomodo
Copy link

@JoachimFlottorp can u actually remove the one line breaking formatting. I am aware I introduced the line change but remove it in this pr and ill merge it. i am happy.

Copy link

@AnatoleAM AnatoleAM left a comment

Choose a reason for hiding this comment

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

This seems good at least for this fork, though I'll note there has been discussion before on adding further network requests on opening user cards, which may be an issue when submitting this to the upstream branch.

src/widgets/dialogs/UserInfoPopup.hpp Show resolved Hide resolved
@AnatoleAM AnatoleAM enabled auto-merge (squash) March 29, 2022 23:25
@TroyKomodo TroyKomodo disabled auto-merge March 29, 2022 23:57
@TroyKomodo
Copy link

@JoachimFlottorp Any updates on this?

@Felanbird
Copy link

@JoachimFlottorp Any updates on this?

I'm perfectly happy to PR the line removal 🙂

@TroyKomodo TroyKomodo merged commit 28cad3d into SevenTV:chatterino7 Apr 1, 2022
TroyKomodo added a commit that referenced this pull request Apr 1, 2022
@Nerixyz Nerixyz mentioned this pull request Apr 20, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants