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

ref: merge TooltipPreviewImage and TooltipWidget #4268

Merged
merged 7 commits into from
Dec 29, 2022

Conversation

kornes
Copy link
Contributor

@kornes kornes commented Dec 28, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

TooltipPreviewImage serves little purpose and depends on TooltipWidget. Since both are singletons doing same thing and used together in codebase, this PR removes TooltipPreviewImage and moves image refreshing logic to TooltipWidget.

changes to merged TooltipWidget:

  • removed unused imports
  • added missing parent to QLabels
  • set Qt::WindowStaysOnTopHint attribute at construction, instead of Qt raise() and windows' HWND_TOPMOST
  • use SignalHolder for 3 connections and change destructor to default one
  • moved adjustSize() (needed for background sizing) to local showEvent()
  • changed external setImage(nullptr) calls to clearImage()
  • setImage() will exit early when passed image matches current one (happens when mouse move over same emote)
  • rename refreshTooltipWidgetPixmap() to refreshPixmap()
  • flattened refreshPixmap()

also found minor bug in current master, where non-animated images doesn't show up in tooltip from first hovered pixel and requires further mouse move. this PR fixes that.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Overall change looks good, good refactor. Just some nitpicks, then I'd like to take this for a spin with some real testing

CHANGELOG.md Show resolved Hide resolved
src/widgets/TooltipWidget.cpp Show resolved Hide resolved
src/widgets/TooltipWidget.cpp Show resolved Hide resolved
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Changes look good, I assume you've tested that things work as expected on Windows, now I just want to ensure things work as expected on Linux X11 and then I'm happy to merge this in

@kornes
Copy link
Contributor Author

kornes commented Dec 29, 2022

pushed fix to bug i overlooked
https://user-images.githubusercontent.com/28986062/209969031-4ec2618b-b560-4406-99a3-2bb20d9b4de3.mp4

Whats wrong
hide and show events wont trigger if 2 chat elements that are showing tooltip are next to eachother with no space between.
When mouse move over to 2nd element, image was correctly replaced, but custom size did not reset (due to no hideEvent) and possible new size of 2nd element was not applied (due to pixmapRefresh being in showEvent)

Fix:

  1. moved pixmap refreshing from showEvent to setImageScale(), early exit if there is no change
  2. always reset custom scale to 0x0 in setImage() with clearImage()

retested everything on this change and found no more issues, im confident its good to go

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@pajlada
Copy link
Member

pajlada commented Dec 29, 2022

Works as expected for me so I'm merging this in, thank you!

@pajlada pajlada merged commit 11fdd7e into Chatterino:master Dec 29, 2022
@kornes kornes deleted the ref_remove-tooltipPreviewImage branch December 29, 2022 15:11
AnatoleAM pushed a commit to SevenTV/chatterino7 that referenced this pull request Jan 29, 2023
* Update qtkeychain to `e5b070831cf1ea3cb98c95f97fcb7439f8d79bd6` (Chatterino#4250)

* Update qtkeychain to `e5b070831cf1ea3cb98c95f97fcb7439f8d79bd6`

* FreeBSD/Cirrus - use system qtkeychain - this ensures we don't have to install pkg-config to get libsecret built

* Set BUILD_SHARED_LIBS to OFF globally - this resolves the libcommuni build issue

* Add Thread Guard for debugging simple threading issues (Chatterino#4254)

* Add ThreadGuard class

* Use ThreadGuard when accessing a ChannelView's messageSnapshot

* Remove protocol from organization domain (Chatterino#4256)

* remove protocol from organization domain

* Add changelog entry

* update changelog entry

Co-authored-by: Sam Heybey <[email protected]>

* Migrate to C++ 20 & switch to websocketpp develop branch (Chatterino#4252)

* feat: c++ 20

* fix: c++ 20 deprecations

* fix(msvc): warnings

* chore: add changelog entry

* fix: formatting

* Update websocketpp to the `develop` branch

* Specify other template type in FlagsEnum != operator

* Remove the user of simple template ids in our websocketpp template class

Also standardizes the file a bit by using nested namespaces, using
pragma once

* fix: turn `MAGIC_MESSAGE_SUFFIX` into a `QString`

* hacky unhacky hacky const char hack

Co-authored-by: Rasmus Karlsson <[email protected]>

* Remove another implicit this-capture for C++20 migration (Chatterino#4257)

* Fix another usage of an implicit this capture

* Add changelog entry

* Enable LTO support for Chatterino builds (Chatterino#4258)

This is disabled by default, and can be enabled by passing `-DCHATTERINO_LTO=On` to your cmake invocation.

* Disable LTO by default (Chatterino#4260)

* SplitContainer refactor (Chatterino#4261)

* Remove unused include util/Helpers.hpp

* SplitContainer::setTag fix parameter naming

* autofy/constify where possible

* More const auto ptr magicifying

* Make SplitNode::Type an enum class

* Move QuickSwitcherPopup includes from header to source file

* Remove unused DropRegion code

* use empty() instead of size() == 0

* Add curly braces everywhere

* Remove useless reinterpret_cast

It was casting Node* to Node*

* Clarify that the connect is QObject::connect

* SplitContainer::setSelected fix parameter naming

* Rename function variables to remove unneccesary underscore

Also move addSpacing parameter out of the layout function

* emplace_back where possible

* Name parameters

* Remove ineffective const from return type

* Make node getters const

* Flatten Node::releaseSplit

* Rename in-function variable to match code style

* [ACTUAL CODE CHANGE/MOVE] Move clamp logic to its own function

* name params

* applyFromDescriptorRecursively: rename node param to baseNode

* [ACTUAL CODE CHANGE/MOVE] Remove the many overloads for append/insertSplit

This utilizes the C++20 designed initializers aggregate initialization feature

* Remove unused includes

* [ACTUAL CODE CHANGE] Clean up dragging logic

There's no need to keep a pointer around to which split is being
dragged, it's already stored in the QDropEvent source()

* UNRELATED .clang-tidy: Only suggest UPPER_CASE for constant global variables

* Remove unused SplitContainer::getSplitCount function

* Use std::max in Node's clamp function

* Remove test code

* DraggedSplit.hpp: remove unused include

* Split `setDraggingSplit` into two functions, `startDraggingSplit` and `stopDraggingSplit`

* Only try to extract images if the image uploader is enabled (Chatterino#4246)

* Only try to extract images if the image uploader is enabled

* Add changelog entry

* Add `qt5-imageformats` to Fedora dependency list (Chatterino#4265)

* Fix text cursor in open channel dialog (Chatterino#4263)

* Fix text cursor not blinking in open channel dialog

* Update CHANGELOG.md

Co-authored-by: pajlada <[email protected]>

* fix: Remove Unused Include Directives (Chatterino#4266)

* fix: remove unused includes

* fix: bad includes

* fix: top include

* fix: streamer mode includes

* fix: missing include

* fix: remove `#else`

Co-authored-by: pajlada <[email protected]>

* Remove unused operators in `Image` (Chatterino#4267)

* ref: merge TooltipPreviewImage and TooltipWidget (Chatterino#4268)

* merge TooltipPreviewImage and TooltipWidget

* changelog

* add empty line before return

* fix signalholder include

* add changelog for bugfix

* fix custom scaling issue

Co-authored-by: pajlada <[email protected]>

* Fix crash that would occur when performing certain actions after removing all tabs (Chatterino#4271)

* Ensure we can deselect notebooks

* Add changelog entry

* Use dynamic_cast instead of raw cast

* Bump cmake/sanitizers-cmake from `99e159e` to `a6748f4` (Chatterino#4274)

Bumps [cmake/sanitizers-cmake](https://github.com/arsenm/sanitizers-cmake) from `99e159e` to `a6748f4`.
- [Release notes](https://github.com/arsenm/sanitizers-cmake/releases)
- [Commits](arsenm/sanitizers-cmake@99e159e...a6748f4)

---
updated-dependencies:
- dependency-name: cmake/sanitizers-cmake
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Refactor SplitHeader class (Chatterino#4276)

* Flatten static functions in anonymous namespace

* SplitHeader ctor: Rename param

* Header: Remove unnecessary `virtual`s

* auto ptr where possible

* Add curly braces

* Comment twitch room modes

* Treat roomModes->slowMode as an integer

* Remove unused `this` from lambdas

* Add `unsigned int` overload for localizeNumbers

* Move thumbnail max age to a constexpr static & set explicit types

* Explicitly use `QObject::connect`

* Use `empty()` instead of `size()`

* Name unused parameters

* Move moderation action refreshing logic from SplitHeader to Split

* Fix crash that could occur when closing down splits (Chatterino#4277)

* Fix crash that could occur when closing down splits

Specifically, if a split was closed after the request for a thumbnail
had been made, but before the request had finished, we'd run into a
use-after-free issue

* Add changelog entry

* Fix crash that could occur when closing down the last of a channel when reloading emotes (Chatterino#4278)

* Fix crash that could occur when a channel is closed

Ensure we copy the QString in case the channel goes out of scope and
we're referring to nothing

* Add changelog entry

* Remove Unnecessary Includes in Headers (Chatterino#4275)

* refactor: remove unnecessary includes in headers

* fix: formatting

* chore: changelog

* fix: scrollbar

* fix: suggestions and old appbase remains

* fix: suggestion

* fix: missing Qt forward declarations

* fix: another qt include

* fix: includes for precompiled-headers=off

* Add missing `<memory>` includes

* Add missing `#pragma once`

* Fix tests

Co-authored-by: Rasmus Karlsson <[email protected]>

* Flatten util/IncognitoBrowser.{h,c}pp (Chatterino#4280)

* Remove Deprecated `QDesktopWidget` (Chatterino#4287)

* fix: remove deprecated `QDesktopWidget`

* chore: add changelog entry

* Fix scrollbar highlight colors when changing message history limit (Chatterino#4288)

* Use correct messagesLimit for Scrollbar

* Update CHANGELOG.md

Co-authored-by: pajlada <[email protected]>

* Fix link to the "connecting to the Twitch IRC server" guide (Chatterino#4293)

* Massage includes some more (Chatterino#4294)

* Add `<functional>` include to QStringHash.hpp

This ensures the base `std::hash` template is declared before this
specialization

* Add missing includes to `src/providers/twitch/TwitchAccountManager.hpp`

* Move explicit HelixChatters constructor to the source file

* Remove unused includes & add used includes to NicknamesModel.hpp

* NicknamesModel.hpp: Remove `virtual` when `override` is used

* Add missing QStringHash include to TwitchEmotes.cpp

* Add missing includes to various files

* Print Qt version in cmake step

Technically unrelated, but I'm sneaking it in

* Add changelog entry

* Bump Cirrus CI FreeBSD image from 12.1 to 13.1 (Chatterino#4295)

* Cirrus CI, print gcc version & print compile commands

* upgrade freebsd from 12.1 to 12.3

* Add changelog entry

* Bump to 13.1

* Update changelog entry

* Fix various small warnings (Chatterino#4296)

* Fix `inconsistent-missing-override` warnings

* Fix `final-dtor-non-final-class` warnings

* Fix `ambiguous-reversed-operator` warnings

* Fix: dont run windeployqt with VCPKG (Chatterino#4297)

* Add feature to select channels to log (Chatterino#4302)

* Add checkbox for custom logging and table with channels to log on Logs page

* Add checkbox to enable and disable logging per channel

* Return from addMessage before logging if custom logging enabled and channel does not have logging enabled

* Use clang-format to fix formatting

* Add CHANGELOG.md entry

* Resolve PR comments

* Remove toggle for channels so any channel listed will be logged

* Move Only log channels listed below checkbox to just above table

* Fix formatting

* Re-order changelog

* ChannelLog constructor: Copy & move instead of const ref & copy

* ChannelLog::createEmpty: Curly brace initialize instead of repeating
name

* ChannelLog toString & createEmpty: nodiscard

* Use COUNT paradigm in model column

* Remove ChanneLoggingModel source file comments

* Use Column::Channel in getRowFromItem

* Rename `getItemFromRow` parameter and mark it as unused

* Curly brace initialize ChannelLog

* private & friend class the model

* Filter out channels to log using a set instead of iterating over a vector every time a message comes in

* Rename `ChannelLog::channel` member to `ChannelLog::channelName`

Also made it private

* mini comment on ChannelLog

Co-authored-by: Felanbird <[email protected]>
Co-authored-by: Rasmus Karlsson <[email protected]>

* Change the highlight order to prioritize Message highlights over User highlights (Chatterino#4303)

* Format YAML and JSON Files With Prettier (Chatterino#4304)

* ci: format yaml and json files with prettier

* chore: add changelog entry

* fix: format everything

* ci: run pretter on all files

* ci: rename prettier step

* Add Felanbird to contributors list (Chatterino#4308)

* Mark big strong contributors as Collaborators (Chatterino#4311)

* Attempt to fix certain ping sound issues on Arch Linux (Chatterino#4285)

* Add Wissididom to contributors list (Chatterino#4312)

* Update `contributors.txt` instructions (Chatterino#4313)

* Fix slash to backslash conversions in url hashes for opening links in incognito (Chatterino#4307)

* Pass link as argument instead of in command string when opening incognito links

* Update CHANGELOG.md

* Make changelog message more user facing

* Remove now unused argument for getCommand

* Update `actionsx/prettier` from `v2` to `e90ec54` (Chatterino#4318)

This finishes the migrations of all workflows that used node12

* Fix the split "Search" menu action not opening the correct search window (Chatterino#4305)

* Fix split menu "Search" action

It now opens a split-specific search instead of a global one

* Add changelog entry

* feat: Add Live Emote Updates for BTTV (Chatterino#4147)

This feature is enabled by default and can be disabled in settings with the "Enable BTTV live emotes updates" setting.

Co-authored-by: Felanbird <[email protected]>
Co-authored-by: pajlada <[email protected]>

* Bump libcommuni version (Chatterino#4320)

This includes a Qt6-related fix that Nerixyz contributed to the main
repo but that hasn't been merged in yet.

* Fix version checking (Chatterino#4329)

https://github.com/Neargye/semver

* Use semver library for version downgrade checking

* Add test validating our current version is valid semver

* Clean up clang-tidy action checks (Chatterino#4331)

* clang-tidy action: Exclude lib & tests dir for clang-tidy CI checks

The lib dir is obvious, but the tests dir is disabled because gtest has
a million different thing that clang-tidy doesn't like, while they're in
reality perfectly reasonable

* clang-tidy-action: Disable the LGTM comment

* Change sound backend from Qt to miniaudio (Chatterino#4334)

Thanks Greenlandicsmiley, Nerixyz, Yoitsu, and helmak for helping debug & test this

* Remove QMediaPlayer includes

* Prefer local path when generating the sound path

* Update changelog entry number

* Disable pitch & spatialization control

* Add CMake Install Support for Windows (Chatterino#4300)

* fix: windows installation with cmake

* fix: support install for all win32 compilers

* chore: add changelog entry

* fix: support `X_VCPKG_APPLOCAL_DEPS_INSTALL`

* chore: document cmake min version

* fix: vcpkg

* fix: plugin path

* fix: remove flattening

* revert: `qt.conf` changes

---------

Co-authored-by: pajlada <[email protected]>

* Disable translation building (Chatterino#4336)

* Use `CMakeDeps` and `CMakeToolchain` as Generators on Conan (Chatterino#4335)

* deps(conan): use `CMakeDeps` as generator

* chore: add changelog entry

* deps(conan): add `CMakeToolchain` generator

* fix: use generated toolchain file

* docs: mention toolchain as well

* fix: spelling

* fix: formatting

* revert: use nmake

* docs: fix documentation

* Remove Qt::Multimedia as a dependency from CMakeLists.txt (Chatterino#4339)

* Remove Multimedia required in find_package

* Remove Multimedia from src/CMakeLists.txt

* Suppress Output of Git Command in CMake (Chatterino#4340)

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: pajlada <[email protected]>
Co-authored-by: Sam Heybey <[email protected]>
Co-authored-by: Felanbird <[email protected]>
Co-authored-by: Douglas <[email protected]>
Co-authored-by: kornes <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Daniel Sage <[email protected]>
Co-authored-by: askepticaldreamer <[email protected]>
Co-authored-by: Thomas Petersen <[email protected]>
Co-authored-by: Wissididom <[email protected]>
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.

2 participants