Better default text formatting #78

MyriaCore · 2020-08-15T20:40:47Z

Closes #75.

Body text shouldn't grow the size of the notification. Instead, it should be cutoff with (...) at about 2-5 lines
- We should probably expose a setting for this too, so the user can choose how many lines they want.
~~The image should be centered vertically by default~~
- ~~There should definitely be a setting for vertically centering the image, or having it in the corner, etc.~~
Deemed out of scope / not enough time to implement. See discussion: Better default text formatting #78 (comment)
There should be configurable margins around the image.

New Config vars

[notification-center]

# Truncates notification bodies with '...' at the specified number of 
# lines. If -1 is specified, the body text will not be truncated.
# Applies only to notifications within the notification center. 
shortenBody = -1

[notification-center-notification-popup]

# The margin around the top, bottom, left, and right of notification
# images. Applies to popup notifications and in-center notifications.
imageMarginTop = 15
imageMarginBottom = 15
imageMarginLeft = 15
imageMarginRight = 0


# Truncates notification bodies with '...' at the specified number of 
# lines. If -1 is specified, the body text will not be truncated.
# Applies only to popup notifications
shortenBody = 5

(Also, I turned the BEGIN_EXAMPLE block into a BEGIN_SRC ini in the readme, so we can have some nice syntax highlighting for the example config :D)

currently set to 5 lines max, but we'll see if it's possible to add customizability later

MyriaCore · 2020-08-15T20:45:33Z

I'm kinda struggling to deal w/ the semi-diverging histories here. 4da6094 shouldn't do anything when applied to here, it's basically just to re-sync my fork. Not sure how to get rid of it lol

MyriaCore · 2020-08-15T21:09:38Z

I thought the elipsization edit would be quick and easy, but I'm getting a weird bug in the notification center with the fonts:

With edit	Without edit

The glade files don't say anything about font sizing, so I assume that this happens somewhere. I don't think it's a config issue. Not sure where it's coming from, though. It looks like the font sizes for the time labels for the notif center and the notif have switched completely.

MyriaCore · 2020-08-24T19:43:52Z

Might be cleaner going through code. There's a labelSetEllipsize function that we could use on the body here. This would also be a perfect place to get the config variable from too.

This reverts commit 585ef02.

MyriaCore · 2020-08-24T20:58:43Z

No, the font swaping is still happening in-center:

This is honestly super strange.

MyriaCore · 2020-08-24T21:13:22Z

Alright, I'm starting to think that this might be some weird bug on my machine, or just some weird bug in general, because I tried testing this with the following edit, and it should literally just be the same code that's currently on master, but it's still giving me this weird new bug.

diff --git a/src/NotificationCenter/Notifications/NotificationPopup.hs b/src/NotificationCenter/Notifications/NotificationPopup.hs
index d4ac9c1..4334372 100644
--- a/src/NotificationCenter/Notifications/NotificationPopup.hs
+++ b/src/NotificationCenter/Notifications/NotificationPopup.hs
@@ -85,13 +85,13 @@ showNotificationWindow config noti dispNotis onClose = do
         dispNotiWithoutHeight
       lblBody = (flip view) dispNoti $ dLabelBody
 
-  labelSetEllipsize lblBody EllipsizeModeEnd 
+  -- labelSetEllipsize lblBody EllipsizeModeEnd 
     -- if configPopupEllipsizeBody config
     -- then EllipsizeModeEnd
     -- else EllipsizeModeNone
 
   -- TODO: Integrate w/ config
-  labelSetLines lblBody 5
+  -- labelSetLines lblBody 5
 
   setWidgetWidthRequest mainWindow $ fromIntegral $ configWidthNoti config

@phuhl I don't wanna bother you, but could you do me a huge favor and try running this on your machine to see if it gives u this bug? I really just wanna move past this rn, and I have no clue where it could be coming from.

phuhl · 2020-08-26T09:23:01Z

Compiles and runs without that wierd bug, but also it does not ellipzise on my machine... Don't know if i am doing smth wrong?

Edit: ahum, wrong branch, nevermind

phuhl · 2020-08-26T09:28:47Z

So, does not work. Same bug as yours. And, coincidentially, shows another bug that I did'nt manage to track down yet, where the content area of the notification center sometimes is too short.

phuhl · 2020-08-26T09:52:25Z

That error exists on the base branch already

phuhl · 2020-08-26T09:58:27Z

Yeah, quite sure, the bug was introduced in d5fd02f which I carelessly merged because I though reading the changes is sufficient. Sorry bout that.

phuhl · 2020-08-26T10:01:28Z

Patch style.css and it works:

 .time {
-    font-size: replaceme0027;
+    font-size: replaceme0028;
 }
 
 .noti-center.time {
-    font-size: replaceme0028;
+    font-size: replaceme0027;
 }

~~Would you mind, introducing the css patch in this PR? Then we could be done with it! @MyriaCore~~ Nevermind, fixed it in master, you just need to merge in the master (of this repo)

MyriaCore · 2020-08-26T16:46:55Z

Prob shouldn't have fixed in master, u should have write perms for this PR. Lemme try to merge the master in.

Merge style.css patch

MyriaCore · 2020-08-26T17:00:37Z

Nice, this patch is working for me too! Thanks for the work @phuhl! Still getting that notification area bug though. That'll probably be for another PR.

…notis

If the max lines specified by the user was not -1, we can infer that they don't want to ellipsize.

…n-center)

WARNING: THIS COMMIT ADDS A KNOWN BUG Getting this bug where none of the action buttons show up on notifications with action buttons. Added some print statements to unify the debugging experience.

MyriaCore · 2020-08-29T20:13:45Z

Alright, I just pushed the changes I made to the glade, and to the code in AbstractNotification.updateNotiContent. This code adds a known bug where notification action buttons never appear, even when specified.

To reproduce this bug:

Send a notification with actions in it. For example:

   notify-send.py "Lorem" "Lorem Ipsum"  \
     -a 'lorem' -i 'text-plain' \
     --action '1:Action One' '2:Action Two' &

Look at output of notification daemon

Bugged output looks something like this:

Number of action buttons after `widgetShowAll`: 2
Number of *visible* action buttons after `widgetShowAll`: 0
Parent of `box_actions` is NOT visible!
Grandparent of `box_actions` is NOT visible!
--------------------------

This bug only happens when the structure of the glade is changed. See the table below for a better understanding about how the structure of the glade has been changed by commit 8405e75:

View Table

Model Heirarchy Before Model View Before

+-------+-------------------+
|       |Title Label        |
| Image |Body Label         |
|       |      buttons      |
|       |    App Icon & Name|
+-------+-------------------+

Model Heirarchy After Model View After

+-------+-------------------+
|       |Title Label        |
| Image |Body Label         |
|       |    App Icon & Name|
+-------+-------------------+
|          buttons          |
+---------------------------+

Ultimately, I really don't know where this bug could be coming from. I can always revert 8405e75 and submit this PR without the improved button box formatting, but if we do want this feature (which we should, since it looks way better IMO), I really need some guidance @phuhl.

MyriaCore · 2020-08-30T16:19:36Z

I think that I do still wanna go through with this, but it's taking too long to implement for the scope of #75. It might make more sense to live as its own being, or as a part of #77, since it'd be the first step to implementing that anyways.

I'll revert this commit and do some last look-overs. Otherwise, this PR is basically done.

This reverts commit 8405e75.

phuhl · 2020-08-31T07:51:09Z

I think, splitting this into two PRs is a good idea! Btw, I also know now, why your buttons don't appear. It is not, because they are hidden, but because of the rather hacky way that I use to set the notification size. Look at line 176 of NotificationPopup.hs. It returns the height of the notification by receiving the preferred height for a set width. I do it like that, because at the point of writing, gtk did not have a feature to set the width directly, only the height. What I did is the following: Get the "preferred height" at the desired width. Set the height to the preferred height and hope, that the width is correct then. That is also the reason why the notification center bugs arround in #40 I think.

Anyways, back to your buttons. I set the height of the notification by setting it on the label in the background (that is the only reason why it is there). Since you changed the glade's layout, the buttons are not taken into account when calculating the height so they stick out to the bottom of the notification. To prove it: return (a + 100) and find that your buttons are totally visible.

phuhl · 2020-08-31T08:04:21Z

Some more pointers here (and also the reasons why I did not implement image align as center by default): When not setting a line limit, it is buggy! I don't know how to fix it. Actually, it seems to be buggy even with shortenBody = 5 but without margins, see second screenshot.

Config:

imageMarginTop = 0
imageMarginBottom = 0
imageMarginLeft = 15
imageMarginRight = 0

shortenBody = -1

imageAlignment = center

phuhl · 2020-08-31T08:08:37Z

I will leave this open for now. I don't know a solution for the vertical centering of the image. But I'd like to know your opinion before moving on.

My concern here is, that we already have a good selection of bugs and I am not sure if it is beneficial to push a broken-ish feature just to have more configuration options. But I also see, that you might be able to hack-config your way around this (which is a terrible UX though).

MyriaCore · 2020-09-01T04:07:26Z

Lol yeah I ran into some weird UX with margins and centered images. I'll look over some of these bugs later

MyriaCore · 2020-09-01T19:07:06Z

Alright, I was able to reproduce some of the bugs you talked about! I have a few notes:

The bug in the image below happens with shortenBody = 5 AND shortenBody = -1, so it appears that this is a bug specifically relating to the vertical centering.
There does seem to be a lot of jank relating to use of margins with use of image centered. I think that these problems basically go away overnight if we just disallow that. So maybe whenever imageAlignment is center, we should ignore the user's preference for imageMarginTop and imageMarginRight`. That won't resolve anything of these bugs, but it'll make for a better UX I think.

I still can't reproduce the first image. I'll work on getting to the bottom of the other one though.

MyriaCore · 2020-09-01T19:20:17Z

I think that the image cutoff one is bugged cause of the getHeight function, as you talked about:

linux_notification_center/src/NotificationCenter/Notifications/NotificationPopup.hs

Lines 173 to 176 in 499c75b

    
           getHeight widget config = do 
        
             (a, b) <- widgetGetPreferredHeightForWidth widget 
        
               $ fromIntegral $ configWidthNoti config 
        
             return a

Could we just directly set the height to getHeight (view dContainer dispNotiWithoutHeight) config?

MyriaCore · 2020-09-01T19:21:00Z

Did you reproduce this with the button swap patch?

phuhl · 2020-09-02T07:55:22Z

Did you reproduce this with the button swap patch?

no, i think this was an the commit after the revert. But it might actually be the same bug, just with more extreme visuals. When the IMG is centered, somehow the container it is centered in behaives as if it was twice as high as the notification or something like that. so, more lines of text -> more offset (I think)

phuhl · 2020-09-02T07:56:28Z

Could we just directly set the height to getHeight (view dContainer dispNotiWithoutHeight) config?

Not sure what you mean...

phuhl · 2020-09-02T07:58:08Z

There does seem to be a lot of jank relating to use of margins with use of image centered.

Not sure if it is due to that, but if it was like that, your proposal would be fine for now.

MyriaCore · 2020-09-02T14:37:16Z

Not sure what you mean...

Not rly sure what I thought I meant, either. I kinda feel like the height setting stuff is where most of our problems come from, so I wanna try to see if I can't just let it do its thing.

I mean, don't Gtk elements naturally grow to encompass everything they need? i might fiddle around w/ it to see if that's where the bug is.

If I can't implement this by the end of the week, I might just gut the vertical alignment bits and just ship the margins & text ellipsization, since those two solve a lot of the problems that I have with the current layout.

MyriaCore · 2020-09-04T14:24:22Z

Didn't get any time, so since it's friday, I'll prob gut the alignment stuff rn and open a new issue for it. It'll just be the ellipsization and margins, which IMO make for an OK experience anyways.

MyriaCore · 2020-09-04T14:59:18Z

@phuhl lmk if any of these bugs persist, or if new bugs appear!

MyriaCore · 2020-09-09T13:23:26Z

thanks for the merge! 🚀

Better default text formatting Former-commit-id: 01f2367

Better default text formatting Former-commit-id: 63d8643 [formerly 63d8643 [formerly 63d8643 [formerly 01f2367]]] Former-commit-id: 392e31b8d990188da11a1806c614901169e1ae98 Former-commit-id: 3766f56fe9076156d839d9e6d9ee93ff11c70e1b Former-commit-id: 03532353a21eb209a698f864751cb09a08218ff7

Added elipsization for body in popup

585ef02

currently set to 5 lines max, but we'll see if it's possible to add customizability later

MyriaCore added 4 commits August 24, 2020 16:03

Revert "Added elipsization for body in popup"

4ae5db5

This reverts commit 585ef02.

Added a config var for ellipsization

1ba8097

Wrote logic for ellipsization and max lines

211a43a

Fixed typo & dependency issues

edf50a1

MyriaCore added 2 commits August 26, 2020 12:55

Merge pull request #6 from phuhl/master

ac8bb8d

Merge style.css patch

Merge branch 'base' into better-text-formatting

9f985a9

MyriaCore added 5 commits August 26, 2020 13:04

Uncommented & integrated config into ellipsization of notif body

8138273

Created config var for max lines in body, & for ellipsization

d3f21ef

Added seperate elipsization and max line config vars for noti center …

4a28f9c

…notis

Refactored elipsization & max-line config vars into a single config var

4656da7

If the max lines specified by the user was not -1, we can infer that they don't want to ellipsize.

Added image alignment config var

6859dfc

MyriaCore mentioned this pull request Aug 26, 2020

Restructure Config Categories #80

Closed

MyriaCore added 5 commits August 26, 2020 16:54

Implemented image alignment configuration for all notis (popups and i…

ccff677

…n-center)

Added new configuration variables to README

977aef3

Added Image margin config vars

ebaa4fb

Fixed typo lol

a60383c

Set img margins from config vars

0325da3

Added some print statements to try to unify the experience

8405e75

WARNING: THIS COMMIT ADDS A KNOWN BUG Getting this bug where none of the action buttons show up on notifications with action buttons. Added some print statements to unify the debugging experience.

MyriaCore added 3 commits August 30, 2020 12:21

Revert "Added some print statements to try to unify the experience"

ac4a399

This reverts commit 8405e75.

Added some syntax highlighting to the readme's example config

3c35c27

Clarified functionality of new config vars in the readme

499c75b

MyriaCore marked this pull request as ready for review August 30, 2020 16:36

This was referenced Aug 30, 2020

Another Feature Request: Buttons in the left or the right. #77

Open

Notification area in the Center is too small, when one of the messages is too long #40

Closed

Removed image alignment configs

ebe9e9d

Almost forgot to remove the image alignment stuff from the readme

864ffa5

This was referenced Sep 4, 2020

Bump version (for AUR and for published releases) #90

Closed

Vertical Centering of Noti Images #91

Open

phuhl merged commit 01f2367 into phuhl:master Sep 9, 2020

ahmubashshir pushed a commit to ahm-forks/linux_notification_center that referenced this pull request Jan 22, 2021

Merge pull request phuhl#78 from MyriaCore/better-text-formatting

63d8643

Better default text formatting Former-commit-id: 01f2367

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better default text formatting #78

Better default text formatting #78

MyriaCore commented Aug 15, 2020 •

edited

Loading

MyriaCore commented Aug 15, 2020

MyriaCore commented Aug 15, 2020 •

edited

Loading

MyriaCore commented Aug 24, 2020

MyriaCore commented Aug 24, 2020

MyriaCore commented Aug 24, 2020

phuhl commented Aug 26, 2020 •

edited

Loading

phuhl commented Aug 26, 2020

phuhl commented Aug 26, 2020

phuhl commented Aug 26, 2020

phuhl commented Aug 26, 2020 •

edited

Loading

MyriaCore commented Aug 26, 2020

MyriaCore commented Aug 26, 2020 •

edited

Loading

MyriaCore commented Aug 29, 2020 •

edited

Loading

MyriaCore commented Aug 30, 2020 •

edited

Loading

phuhl commented Aug 31, 2020

phuhl commented Aug 31, 2020

phuhl commented Aug 31, 2020

MyriaCore commented Sep 1, 2020

MyriaCore commented Sep 1, 2020

MyriaCore commented Sep 1, 2020

MyriaCore commented Sep 1, 2020

phuhl commented Sep 2, 2020

phuhl commented Sep 2, 2020

phuhl commented Sep 2, 2020

MyriaCore commented Sep 2, 2020

MyriaCore commented Sep 4, 2020

MyriaCore commented Sep 4, 2020

MyriaCore commented Sep 9, 2020

Better default text formatting #78

Better default text formatting #78

Conversation

MyriaCore commented Aug 15, 2020 • edited Loading

New Config vars

MyriaCore commented Aug 15, 2020

MyriaCore commented Aug 15, 2020 • edited Loading

MyriaCore commented Aug 24, 2020

MyriaCore commented Aug 24, 2020

MyriaCore commented Aug 24, 2020

phuhl commented Aug 26, 2020 • edited Loading

phuhl commented Aug 26, 2020

phuhl commented Aug 26, 2020

phuhl commented Aug 26, 2020

phuhl commented Aug 26, 2020 • edited Loading

MyriaCore commented Aug 26, 2020

MyriaCore commented Aug 26, 2020 • edited Loading

MyriaCore commented Aug 29, 2020 • edited Loading

MyriaCore commented Aug 30, 2020 • edited Loading

phuhl commented Aug 31, 2020

phuhl commented Aug 31, 2020

phuhl commented Aug 31, 2020

MyriaCore commented Sep 1, 2020

MyriaCore commented Sep 1, 2020

MyriaCore commented Sep 1, 2020

MyriaCore commented Sep 1, 2020

phuhl commented Sep 2, 2020

phuhl commented Sep 2, 2020

phuhl commented Sep 2, 2020

MyriaCore commented Sep 2, 2020

MyriaCore commented Sep 4, 2020

MyriaCore commented Sep 4, 2020

MyriaCore commented Sep 9, 2020

MyriaCore commented Aug 15, 2020 •

edited

Loading

MyriaCore commented Aug 15, 2020 •

edited

Loading

phuhl commented Aug 26, 2020 •

edited

Loading

phuhl commented Aug 26, 2020 •

edited

Loading

MyriaCore commented Aug 26, 2020 •

edited

Loading

MyriaCore commented Aug 29, 2020 •

edited

Loading

MyriaCore commented Aug 30, 2020 •

edited

Loading