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

Set appropriate window application and titles instead of defaulting to gsconnect #1875

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sidevesh
Copy link
Contributor

No description provided.

@sidevesh sidevesh changed the title Set appropriate window titles instead of defaulting to gsconnect or GSConnect Set appropriate window application and titles instead of defaulting to gsconnect or GSConnect Oct 13, 2024
@sidevesh sidevesh changed the title Set appropriate window application and titles instead of defaulting to gsconnect or GSConnect Set appropriate window application and titles instead of defaulting to gsconnect Oct 13, 2024
Copy link
Member

@ferdnyc ferdnyc left a comment

Choose a reason for hiding this comment

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

I'm not at all sold on most of these. They appear to be solutions in search of a problem, and many are redundant with the UI template definitions. (Redundancy is bad.) -1 from me without better justification for why it's necessary.

@@ -40,6 +40,7 @@ const Dialog = GObject.registerClass({
_init(params) {
super._init({
application: Gio.Application.get_default(),
title: _('Send SMS'),
Copy link
Member

Choose a reason for hiding this comment

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

This is already set in the template, setting it here is redundant and makes maintenance harder:

<property name="title" translatable="yes">Send SMS</property>

super._init(params);
super._init(Object.assign({
application: Gio.Application.get_default(),
title: _('Messaging'),
Copy link
Member

Choose a reason for hiding this comment

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

This is already set in the template, setting it here is redundant and makes maintenance harder:

<property name="title" translatable="yes">Messaging</property>

@@ -100,6 +100,8 @@ export const InputDialog = GObject.registerClass({

_init(params) {
super._init(Object.assign({
application: Gio.Application.get_default(),
title: _('Remote Input'),
Copy link
Member

Choose a reason for hiding this comment

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

This is set by defining the headerbar.title, which is the title of a GtkDialog in every way that matters.

In the application dash and overview:

image image

In the Gtk Inspector:

image

image

What issue are you trying to solve, by also passing a title parameter to the constructor?

@@ -45,6 +45,7 @@ const ReplyDialog = GObject.registerClass({
_init(params) {
super._init({
application: Gio.Application.get_default(),
title: _('Reply'),
Copy link
Member

Choose a reason for hiding this comment

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

In this case, headerbar.title is set to params.notification.appName, which will be the title of the dialog in every way that matters. What's the purpose of also setting it generically, here?

@@ -53,6 +53,7 @@ export const DeviceChooser = GObject.registerClass({
super._init({
use_header_bar: true,
application: Gio.Application.get_default(),
title: params.title,
Copy link
Member

Choose a reason for hiding this comment

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

title is already set in the template, as Select a Device, which is why params.title is set as the headerbar subtitle:

<property name="title" translatable="yes">Select a Device</property>

@@ -144,6 +144,8 @@ const Dialog = GObject.registerClass({
}, class Dialog extends Gtk.MessageDialog {
_init(params) {
super._init({
application: Gio.Application.get_default(),
title: _('Ringing'),
Copy link
Member

Choose a reason for hiding this comment

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

This dialog is a mess, and doesn't appear to have a title at all. So I'll probably give you this one. But really it should probably be redone with a UI template definition, like basically every other window in the code.

(Actually now that notifications can have persistent action buttons (FINALLY!), it might make way more sense to replace the entire dialog with an urgent notification that has a "Silence" or "Acknowledge" action button embedded.)

@ferdnyc
Copy link
Member

ferdnyc commented Oct 17, 2024

The tests are failing in the CI because you added Gio.Application.get_default() calls to modules where Gio isn't imported, BTW.

@sidevesh
Copy link
Contributor Author

Sorry for the messy changes, I was trying to solve the issue of generic gsconnect icon and name in the dash,
I agree the addition of title in almost all the places is not useful, I will remove those,
are you okay with passing the default application to constructor so that it shows those windows as gsconnect app instead of the generic icon in the dash ?

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