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

Option to display non-critical notifications over full-screen apps #10182

Closed
wants to merge 4 commits into from
Closed

Option to display non-critical notifications over full-screen apps #10182

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 27, 2021

#9529

Seems okay here... this is probably not quite ready to merge yet though, because this change requires adding display-noncritical-in-fullscreen to org.cinnamon.desktop.notifications, and I could not for the life of me figure out how to programmatically add or update this in the cinnamon repo.

For testing I manually edited /usr/share/glib-2.0/schemas/org.cinnamon.desktop.notifications.gschema.xml to add the new key, and recompiled using glib-compile-schemas. If you'd like to merge then please make sure to update things on your end so the new key can be properly added.

@Odyseus
Copy link
Contributor

Odyseus commented Jun 27, 2021

Hello, @Ammako.

...and I could not for the life of me figure out how to programmatically add or update this in the cinnamon repo.

The schemas you are looking for are in the cinnamon-desktop package. Specifically the org.cinnamon.desktop.notifications.gschema.xml.in.in file. You could create a PR there and reference it here (and vice versa).

An observation on your JavaScript changes. You are using tabs in your changes, when the general convention is spaces.

@ghost
Copy link
Author

ghost commented Jun 27, 2021

I used spaces, but I think the github web editor might have been dumb and converted some of it to tabs (I was too lazy to set up git for direct push.)

Managed to mess up some of my indentation though, and some other parts of messageTray.js were using tabs already. I left them as-is because I figured it would be best if I didn't touch unrelated parts of the code, but if everything needs to be spaces then I might as well go ahead and fix that too.

@@ -1014,6 +1015,8 @@ MessageTray.prototype = {
this._notification.actor._parent_container.remove_actor(this._notification.actor);
}

let canShowNonCriticalInFullscreen = this._nonCriticalDisplayFullscreen;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not aligned with the other lines

Copy link
Author

Choose a reason for hiding this comment

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

So annoying lol, I swear it was aligned when I initially made the change. No idea what happened.

This pull request was closed.
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