Remember Popped-up Chat Size #5635

maliByatzes · 2024-10-08T13:29:57Z

Fixes #1788
Adedd two new values in the Settings.hpp file to keep track of the last popup window's width and height. On closing a popup window, while the main window is still active, both lastPopupWidth and lastPopupHeight are updated with the last window's dimensions. When a new popup window is created it uses those same dimensions to be same width and height as the last window.

Nerixyz

(discussion): Should we only save the dimensions? I feel like it would be nice to save the position as well (QSize ⇾ QRect). We have functions to do bounds-checking already, so I don't see why not.

Nerixyz · 2024-10-08T15:17:28Z

src/widgets/Window.cpp

+        this->resize(int(getSettings()->lastPopupWidth.getValue() * this->scale()),
+                     int(getSettings()->lastPopupHeight.getValue() * this->scale())


I think this shouldn't multiply by the scale, since that affects the size. The scenario would be:

Open popup

Resize to desired size

Close popup again

Zoom in/out in main window

Open popup

[What's the expected size here?] - I'd say it should have the same size as before

Nerixyz · 2024-10-08T15:27:10Z

src/singletons/Settings.hpp

+    IntSetting lastPopupWidth = {"/appearance/popup/lastWidth", 300};
+    IntSetting lastPopupHeight = {"/appearance/popup/lastHeight", 500};


Ideally, this should save a QSize. That would require a specialization for pajlada::[De]Serialize like for HighlightBadge.

If you're up for it, this would be a nice addition

pajlada · 2024-10-08T15:39:35Z

(discussion): Should we only save the dimensions?

imo yeah, at least as part of this change. keeping it simple

Nerixyz · 2024-10-08T15:43:28Z

imo yeah, at least as part of this change. keeping it simple

Sure, but then it should be "forwards" compatible with settings that save the position as well (i.e. save the stuff in an object with "width" and "height" - I think it should be enough to rename the keys to /appearance/lastPopup/width as that would create an object).

pajlada · 2024-10-08T16:56:19Z

Thank you @maliByatzes!

Two open things:

As Nerixyz mentioned in Remember Popped-up Chat Size #5635 (comment) - If you're up for it, you can change the setting to be a ChatterinoSetting<QSize>, meaning you'd need to implement a custom serializer/deserializer as in

chatterino2/src/controllers/highlights/HighlightBadge.hpp

Lines 74 to 136 in afa8067

    
           namespace pajlada { 
        
           template <> 
        
           struct Serialize<chatterino::HighlightBadge> { 
        
               static rapidjson::Value get(const chatterino::HighlightBadge &value, 
        
                                           rapidjson::Document::AllocatorType &a) 
        
               { 
        
                   rapidjson::Value ret(rapidjson::kObjectType); 
        
                   chatterino::rj::set(ret, "name", value.badgeName(), a); 
        
                   chatterino::rj::set(ret, "displayName", value.displayName(), a); 
        
                   chatterino::rj::set(ret, "showInMentions", value.showInMentions(), a); 
        
                   chatterino::rj::set(ret, "alert", value.hasAlert(), a); 
        
                   chatterino::rj::set(ret, "sound", value.hasSound(), a); 
        
                   chatterino::rj::set(ret, "soundUrl", value.getSoundUrl().toString(), a); 
        
                   chatterino::rj::set(ret, "color", 
        
                                       value.getColor()->name(QColor::HexArgb), a); 
        
                   return ret; 
        
               } 
        
           }; 
        
           template <> 
        
           struct Deserialize<chatterino::HighlightBadge> { 
        
               static chatterino::HighlightBadge get(const rapidjson::Value &value, 
        
                                                     bool *error) 
        
               { 
        
                   if (!value.IsObject()) 
        
                   { 
        
                       PAJLADA_REPORT_ERROR(error); 
        
                       return chatterino::HighlightBadge(QString(), QString(), false, 
        
                                                         false, false, "", QColor()); 
        
                   } 
        
                   QString _name; 
        
                   QString _displayName; 
        
                   bool _showInMentions = false; 
        
                   bool _hasAlert = true; 
        
                   bool _hasSound = false; 
        
                   QString _soundUrl; 
        
                   QString encodedColor; 
        
                   chatterino::rj::getSafe(value, "name", _name); 
        
                   chatterino::rj::getSafe(value, "displayName", _displayName); 
        
                   chatterino::rj::getSafe(value, "showInMentions", _showInMentions); 
        
                   chatterino::rj::getSafe(value, "alert", _hasAlert); 
        
                   chatterino::rj::getSafe(value, "sound", _hasSound); 
        
                   chatterino::rj::getSafe(value, "soundUrl", _soundUrl); 
        
                   chatterino::rj::getSafe(value, "color", encodedColor); 
        
                   auto _color = QColor(encodedColor); 
        
                   if (!_color.isValid()) 
        
                   { 
        
                       _color = chatterino::HighlightBadge::FALLBACK_HIGHLIGHT_COLOR; 
        
                   } 
        
                   return chatterino::HighlightBadge(_name, _displayName, _showInMentions, 
        
                                                     _hasAlert, _hasSound, _soundUrl, 
        
                                                     _color); 
        
               } 
        
           }; 
        
           }  // namespace pajlada

- if this is something you'd rather not do, let me know and I'll help you implement it in this PR

Look at the scaling thing Nerixyz mentioned in Remember Popped-up Chat Size #5635 (comment) and ensure the size is retained as expected

github-actions

clang-tidy made some suggestions

github-actions · 2024-10-08T16:58:49Z

src/widgets/Window.cpp

@@ -75,7 +75,9 @@ Window::Window(WindowType type, QWidget *parent)
    }
    else
    {
-        this->resize(int(300 * this->scale()), int(500 * this->scale()));
+        this->resize(int(getSettings()->lastPopupWidth.getValue() * this->scale()),


warning: narrowing conversion from 'int' to 'float' [bugprone-narrowing-conversions]

this->resize(int(getSettings()->lastPopupWidth.getValue() * this->scale()), ^

github-actions · 2024-10-08T16:58:49Z

src/widgets/Window.cpp

@@ -75,7 +75,9 @@
    }
    else
    {
-        this->resize(int(300 * this->scale()), int(500 * this->scale()));
+        this->resize(int(getSettings()->lastPopupWidth.getValue() * this->scale()),
+                     int(getSettings()->lastPopupHeight.getValue() * this->scale())


warning: narrowing conversion from 'int' to 'float' [bugprone-narrowing-conversions]

int(getSettings()->lastPopupHeight.getValue() * this->scale()) ^

maliByatzes · 2024-10-08T20:16:26Z

Thank you @maliByatzes!

Two open things:

As Nerixyz mentioned in Remember Popped-up Chat Size #5635 (comment) - If you're up for it, you can change the setting to be a ChatterinoSetting<QSize>, meaning you'd need to implement a custom serializer/deserializer as in

chatterino2/src/controllers/highlights/HighlightBadge.hpp

Lines 74 to 136 in afa8067

namespace pajlada {

template <>

struct Serialize<chatterino::HighlightBadge> {

static rapidjson::Value get(const chatterino::HighlightBadge &value,

rapidjson::Document::AllocatorType &a)

{

rapidjson::Value ret(rapidjson::kObjectType);

chatterino::rj::set(ret, "name", value.badgeName(), a);

chatterino::rj::set(ret, "displayName", value.displayName(), a);

chatterino::rj::set(ret, "showInMentions", value.showInMentions(), a);

chatterino::rj::set(ret, "alert", value.hasAlert(), a);

chatterino::rj::set(ret, "sound", value.hasSound(), a);

chatterino::rj::set(ret, "soundUrl", value.getSoundUrl().toString(), a);

chatterino::rj::set(ret, "color",

value.getColor()->name(QColor::HexArgb), a);

return ret;

}

};

template <>

struct Deserialize<chatterino::HighlightBadge> {

static chatterino::HighlightBadge get(const rapidjson::Value &value,

bool *error)

{

if (!value.IsObject())

{

PAJLADA_REPORT_ERROR(error);

return chatterino::HighlightBadge(QString(), QString(), false,

false, false, "", QColor());

}

QString _name;

QString _displayName;

bool _showInMentions = false;

bool _hasAlert = true;

bool _hasSound = false;

QString _soundUrl;

QString encodedColor;

chatterino::rj::getSafe(value, "name", _name);

chatterino::rj::getSafe(value, "displayName", _displayName);

chatterino::rj::getSafe(value, "showInMentions", _showInMentions);

chatterino::rj::getSafe(value, "alert", _hasAlert);

chatterino::rj::getSafe(value, "sound", _hasSound);

chatterino::rj::getSafe(value, "soundUrl", _soundUrl);

chatterino::rj::getSafe(value, "color", encodedColor);

auto _color = QColor(encodedColor);

if (!_color.isValid())

{

_color = chatterino::HighlightBadge::FALLBACK_HIGHLIGHT_COLOR;

}

return chatterino::HighlightBadge(_name, _displayName, _showInMentions,

_hasAlert, _hasSound, _soundUrl,

_color);

}

};

} // namespace pajlada

if this is something you'd rather not do, let me know and I'll help you implement it in this PR

Look at the scaling thing Nerixyz mentioned in Remember Popped-up Chat Size #5635 (comment) and ensure the size is retained as expected

Thank you for your feedback, I'll look into these suggested changes, I will scream for help if I can't figure it out.

maliByatzes · 2024-10-09T05:22:26Z

@pajlada I wrote the below Serialize and Deserialize for QSize, I'm wondering if it's corrects?

#pragma once

#include <pajlada/serialize.hpp>
#include <QSize>

namespace pajlada {

template <>
struct Serialize<QSize> {
    static rapidjson::Value get(const QSize &value,
                                rapidjson::Document::AllocatorType &a)
    {
        rapidjson::Value ret(rapidjson::kObjectType);

        chatterino::rj::set(ret, "width", value.width(), a);
        chatterino::rj::set(ret, "height", value.height(), a);

        return ret;
    }
};

template <>
struct Deserialize<QSize> {
    static QSize get(const rapidjson::Value &value, bool *error)
    {
        if (!value.IsObject())
        {
            PAJLADA_REPORT_ERROR(error);
            return QSize{};
        }

        int _width;
        int _height;

        chatterino::rj::getSafe(value, "width", _width);
        chatterino::rj::getSafe(value, "height", _height);

        return QSize(_width, _height);
    }
};

}  // namespace pajlada

pajlada · 2024-10-09T09:12:09Z

@pajlada I wrote the below Serialize and Deserialize for QSize, I'm wondering if it's corrects?

That looks correct at a glance yeah!

maliByatzes · 2024-10-09T18:29:50Z

@pajlada Im having a problem with the changes I made above, the lastPopup always return 300 and 500 it is not updated. So im wondering if the .getValue() method is a copy not a reference or it is just not updated at all.

pajlada · 2024-10-09T21:33:35Z

Im having a problem with the changes I made above, the lastPopup always return 300 and 500 it is not updated. So im wondering if the .getValue() method is a copy not a reference or it is just not updated at all.

Yeah you're right - getValue returns a copy, not a reference.
So at 5755dc0 (#5635) you need to create a QSize then call setValue on the setting with the QSize you just created

maliByatzes · 2024-10-10T15:48:12Z

@pajlada A call to setValue is returning the below error:

/home/malib/software/chatterino2/lib/serialize/include/pajlada/serialize/serialize.hpp:36:17: error: no matching function for call to ‘rapidjson::GenericValue<rapidjson:
:UTF8<> >::GenericValue(const QSize&)’                                                                                                                                   
   36 |         RJValue ret(value);                                                                                                                                      
      |                 ^~~

with the below snippet using setValue

QRect rect = this->getBounds();                                                                                                                                  
QSize newSize(rect.width(), rect.height());                                                                                                                      
getSettings()->lastPopupSetting.setValue(newSize);

pajlada · 2024-10-10T16:01:04Z

@maliByatzes Any place that sets lastPopupSetting will need to include the new file you made (RapidJsonSerializeQSize) so the serialize library can find the implementation

It could be included in Settings.hpp like the QString variant, but because this is so rarely used, I think I'd prefer if it's only included in Window.cpp

there's a chance we have to include it in Settings.hpp, but try it in Window.cpp first

maliByatzes · 2024-10-10T16:32:16Z

@pajlada Submitted the last commit for this PR, but I was thinking I could also add the position tracking as @/Nerixyz said earlier. Just to keep it simple.

pajlada · 2024-10-10T17:38:23Z

I'd prefer to not do position tracking for now, since I'm not sure it's something we want

github-actions

clang-tidy made some suggestions

src/util/RapidJsonSerializeQSize.hpp

pajlada

Thank you! As a first-time contributor, you are now eligible to be listed in the About page in Chatterino.

If you want this, you can open a new PR where you modify the resources/contributors.txt file and add yourself to the list. Read the comments at the top for instructions.

Let me know if you have any questions about the follow-up commits I made to your code

Keep track of popup window size

919350a

Nerixyz reviewed Oct 8, 2024

View reviewed changes

github-actions bot reviewed Oct 8, 2024

View reviewed changes

Use QSize for dimensions

5755dc0

Finish up last popup setting

a703be9

github-actions bot reviewed Oct 10, 2024

View reviewed changes

pajlada added 12 commits October 12, 2024 11:43

nit: remove unused file selected

e7fa51b

nit: change path of last popup size to /appearance/lastPopup/size

bc8fb22

nit: reformat lastPopupSetting

0025788

nit: use QSizeSetting alias over ChatterinoSetting<QSize>

262b101

nit: remove underscore from width/height in Deserialize<QSize>

7f04d41

fix: handle invalid width/height values in the setting

bf9b4ba

Handle invalid settings

c7ce348

nit: silence clang-tidy's "missing chatterino" complaining

8e10167

nit: silence clang-tidy's complaining about repeating return type

00e99fe

nit: rename setting variable to lastPopupSize

6d00c26

fix: formatting

2798407

chore: add changelog entry

011d4da

Merge branch 'master' into ft-popup-window

c7ac0fd

pajlada approved these changes Oct 12, 2024

View reviewed changes

pajlada enabled auto-merge (squash) October 12, 2024 10:10

pajlada merged commit 2d818a7 into Chatterino:master Oct 12, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remember Popped-up Chat Size #5635

Remember Popped-up Chat Size #5635

maliByatzes commented Oct 8, 2024

Nerixyz left a comment

Nerixyz Oct 8, 2024

Nerixyz Oct 8, 2024

pajlada Oct 8, 2024

pajlada commented Oct 8, 2024

Nerixyz commented Oct 8, 2024

pajlada commented Oct 8, 2024

github-actions bot left a comment

github-actions bot Oct 8, 2024

github-actions bot Oct 8, 2024

maliByatzes commented Oct 8, 2024

maliByatzes commented Oct 9, 2024 •

edited

Loading

pajlada commented Oct 9, 2024

maliByatzes commented Oct 9, 2024 •

edited

Loading

pajlada commented Oct 9, 2024

maliByatzes commented Oct 10, 2024

pajlada commented Oct 10, 2024 •

edited

Loading

maliByatzes commented Oct 10, 2024

pajlada commented Oct 10, 2024

github-actions bot left a comment

pajlada left a comment

		this->resize(int(getSettings()->lastPopupWidth.getValue() * this->scale()),
		int(getSettings()->lastPopupHeight.getValue() * this->scale())

		IntSetting lastPopupWidth = {"/appearance/popup/lastWidth", 300};
		IntSetting lastPopupHeight = {"/appearance/popup/lastHeight", 500};

Remember Popped-up Chat Size #5635

Remember Popped-up Chat Size #5635

Conversation

maliByatzes commented Oct 8, 2024

Nerixyz left a comment

Choose a reason for hiding this comment

Nerixyz Oct 8, 2024

Choose a reason for hiding this comment

Nerixyz Oct 8, 2024

Choose a reason for hiding this comment

pajlada Oct 8, 2024

Choose a reason for hiding this comment

pajlada commented Oct 8, 2024

Nerixyz commented Oct 8, 2024

pajlada commented Oct 8, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Oct 8, 2024

Choose a reason for hiding this comment

github-actions bot Oct 8, 2024

Choose a reason for hiding this comment

maliByatzes commented Oct 8, 2024

maliByatzes commented Oct 9, 2024 • edited Loading

pajlada commented Oct 9, 2024

maliByatzes commented Oct 9, 2024 • edited Loading

pajlada commented Oct 9, 2024

maliByatzes commented Oct 10, 2024

pajlada commented Oct 10, 2024 • edited Loading

maliByatzes commented Oct 10, 2024

pajlada commented Oct 10, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

pajlada left a comment

Choose a reason for hiding this comment

maliByatzes commented Oct 9, 2024 •

edited

Loading

maliByatzes commented Oct 9, 2024 •

edited

Loading

pajlada commented Oct 10, 2024 •

edited

Loading