Widget Controls

[orionchikby]

Hi. For myself, I added the ability to control buttons. I didn’t really understand the specifics of writing extensions, so I probably didn’t write very well. But how can an idea come in handy.

[Moon-0xff]

Hi @orionchikby
Are you using a touchscreen or another "alternative" for a mouse?

[orionchikby]

@Moon-0xff no. only a mouse on a laptop. I haven't tested much.

[Moon-0xff]

I'm not sure how "justified" is this change when we already implemented the mouse controls, but I know this is a great change for people not using a mouse, and/or without thumb buttons for next/prev.

I have been thinking of adding just the next/prev buttons at each side of the label like this: << (label) >>, discarding the pause button. What do you think?

[orionchikby]

In a normal situation, I need a pause and a forward button. And where they will be located is already better for you to decide. It's just more convenient for me to see all the buttons at once on the right. I think buttons are optional (not so important) for your extension

[Moon-0xff]

They are kinda optional, but we already implemented them and widened the scope of the extension, so this to me looks like a sensible change to add (maybe even an obvious change to add)

It's just more convenient for me to see all the buttons at once on the right

Yeah, that sounds right.

[Batwam]

Hi everyone! the code was a bit broken on my end as I have the label on the right so I have suggested some corrections above.

With optional controls, the user would have the ability to allocate left/middle/right click to other actions like open menu, next player or open app, which could be useful for some, especially if they don't have backward/forward buttons on their mouse (or use a trackpad).

If we are going to integrate this, I would recommend making it a bit more compact. For instance, add , style: 'padding: 0px;' to button.set_child(). It then looks like this, from to (and yes, there is an issue with the Dropbox systray icon inserting itself between the label and controls)

Or, would you prefer to combine all the controls in a single "pill" like this within this.box without track_over to be consistent with the quick menu?

This looks neater to me and prevents the Dropbox icon from inserting itself... The only downside is that a left click will also trigger the left click action defined for the extension so if controls are enabled, we might need to disable left click but you get 3 controls for the price of one button so it's not a bad trade-off in my books.

(@Moon-0xff by the way, we should probably include none in the click drop down options before the next release) (edit: done)

[orionchikby]

the idea of disabling clicking (add none) and moving everything into a box is a good.

[Batwam]

yep. none is now in the list of options as of last night so just update your main. The alternative would be to find a way to separate left click on the label and left click on the controls (I haven't found how).

[Batwam]

You might want to use this.box.add_child (check the existing code) and disable the left click unless there is a way to distinguish a left click on the label Vs left click on the controls.

You might also want to check that it still works with icon right/left/none just in case.

[Moon-0xff]

Overall I'm pretty satisfied with your changes, most of the changes i pushed now are about clarity which is more of an stylistically choice honestly.

edit: I also broke the extension with the last commit but I'm working through a fix right now

[Batwam]

How about using the same existing this.box rather than creating a separate this.container?

[Moon-0xff]

I think the only things left to do is:

• Connect show-panel-controls option so there's no need to reload the shell
• Properly hide the controls when the label is also hidden
• Attach the controls to this.box if we want to do that

The last two items might be quite challenging

edit: Also, the building process of the buttons is quite slow for some reason and it looks like in the current state introduces some tiny shell freezes (my fault, i think).

[Moon-0xff]

How about using the same existing this.box rather than creating a separate this.container?

Might be challenging, and a matter of preference

[Batwam]

It's a one liner, just replace container.insert_child_at_index with this.box.insert_child and turn off the hover thingy. Also need to disable left click action though.

I did previously, you can see it in the screenshot shot I posted in my earlier comments. It also solves Dropbox inserting itself but you lose the left click action.

[Moon-0xff]

I foresee a bunch of problems that might provoke, but that might be just pessimism. Feel free to share a diff
($ git diff > diff.diff)

[Batwam]

Yeah, it looks neater but we might need to check against a few edge cases, including icon left/right and so on.

[Moon-0xff]

Also dropping the left click action shouldn't be necessary and we should seek a better approach.

[Batwam]

Ok, let's drop the idea then.

[Moon-0xff]

@orionchikby I suggest you to split your changes into more commits, this simplifies the reviewing process, makes the intent of changes more obvious, and takes better advantage of git altogether.

[orionchikby]

@Moon-0xff sorry, I will answer with one comment. I don't think you should take my request, because it changes the logic of your widget 1 showControlComboBox - bad merge, my mistake 2 I don't understand the coding style I am using the standard, built into phpstorm which complies with https://standardjs.com/rules.html Not using {} is a big pain for me. I'm so used to it that it's very hard to give them up. besides, such blocks are more readable, but this is just my opinion. 3 REFRESH_RATE already exist , and i got the error 4 catch - not needed if everything works well. but since he did not log errors, I could not catch them

thanks for your extension

[Batwam]

See gjs guide here: https://gjs.guide/guides/gjs/style-guide.html#rule-list

This page is silent on if syntax but does provide some inputs on other aspects.

From these examples, single line if statements don't have {} but they do end each line with ; which we haven't been doing.

if (menu instanceof Gtk.Menu)
        log('it emitted it with a GtkMenu argument!');

This Gnome guide does the same.

I have to say that it's been giving me some headaches, especially when I insert extra line for testing logs and I'm not used to this syntax either but that seems to be the way Gnome devs want us to write. It's more compact I guess.

[orionchikby]

It's strange for me. But apparently they know better. I don't like to skimp on curly braces.

[Moon-0xff]

This branch is too old to merge well into main. However in the future, after we finish the refreshless implementation I want to try to add control buttons again, and I will probably use this PR as reference.

So, although this PR will not be merged, it's a great contribution regardless. Thanks @orionchikby !