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

Bring "Catts" in-tree #1234

Merged
merged 30 commits into from
Nov 2, 2021
Merged

Bring "Catts" in-tree #1234

merged 30 commits into from
Nov 2, 2021

Conversation

davidmhewitt
Copy link
Member

See #1232

Currently, this is just the bare minimum to get this working in-tree instead of as a plugin. I'll spend the next little while cleaning up the code-style and looking into some of the issues with this. So just opened as a draft for now.

@davidmhewitt
Copy link
Member Author

cc @aral

Some of my commits here may be useful for you to port back into Catts while we're waiting for any feedback from the core team.

Currently the only relevant one is c1d5871 . You can drop the dependency on Wnck entirely as the window manager is perfectly capable of giving you title of the window in a non X11 specific way.

@aral
Copy link

aral commented Aug 29, 2021

Some of my commits here may be useful for you to port back into Catts

Thanks, David. These look great. Being new to the platform, I wish I had the namespace declarations when I was trying to port it to Odin; would have made it easier to figure what what was from where :)

Will look into incorporating the ones you recommend back into Catts and fingers crossed Catts won’t need to exist for much longer. Will be a good day when I can hit the archive button on the repository :)

Update: removed Wnck dependency as per your commit (small-tech/catts@6769514)

@aral
Copy link

aral commented Aug 29, 2021

@davidmhewitt Just released Catts 1.0.1 with your escape fix. Thanks again :)

@aral
Copy link

aral commented Aug 30, 2021

@davidmhewitt Regarding your HiDPI support commit, is there a way I can get a reference to InternalUtils from the plugin to port these changes back to Catts? (Or is there a public API plugins can use for the same purpose?)

@davidmhewitt
Copy link
Member Author

@aral Utils.get_ui_scaling_factor should be the one

@aral
Copy link

aral commented Aug 30, 2021

@aral Utils.get_ui_scaling_factor should be the one

Thank you :)

@cassidyjames
Copy link
Contributor

cassidyjames commented Sep 2, 2021

Couple of quick notes:

  • we'll need to figure out the exact styling, but the current pure white with no shadow doesn't work well e.g. with a white window behind it cc @elementary/ux

  • Alt+Tabbing with a single window (or no windows) should emit the GLib.beep thud sound like the current implementation

Otherwise I think this is a great start at a traditional window switcher.

@ChildishGiant
Copy link

I think that the switcher should appear on whichever screen is active (has the cursor on). Currently I can be using my second monitor and then when I try to alt-tab I think it's not worked, only to remember that it is on the other screen.

@cassidyjames cassidyjames added the Needs Design Waiting for input from the UX team label Sep 3, 2021
@aral
Copy link

aral commented Sep 4, 2021

we'll need to figure out the exact styling, but the current pure white with no shadow doesn't work well e.g. with a white window behind it cc @elementary/ux

Also, please see my commits linked to from #1234 (comment) as the hidpi commit here changes the appearance of the switcher and those commits both make the icon size consistent with Wingpanel (apart from @1x, where I implemented an optical adjustment to make the icons larger for better legibility/usability – the latter adjustment is in a separate commit to make it easier to cherry pick.)

@davidmhewitt
Copy link
Member Author

davidmhewitt commented Sep 5, 2021

@aral I've updated the calculations for positioning and sizing based on your changes, but have left the icons sizes and padding as is for now.

64px feels a little small for both HiDPI and LoDPI for me, and we generally don't make any allowances anywhere for things in HiDPI to be a different size to its LoDPI counterpart. Any differences you see there will be purely down to the differing DPI of your two displays, and we don't make allowances for that, since everyone's display is different, we simply scale by the scale factor.

However, it's a shame that 96px feels like the "right" size, as we don't ship 96px icons, so displaying them at this size means they aren't aligned to the pixel grid and will look blurry relative to displaying them at 64px or 128px.

I've added the beep sound when switching isn't possible, will wait from further feedback from @elementary/ux regarding design/sizing.

Edit: CI is failing because of #1243 , it is not the fault of this branch. Also, merging master into this branch will break it.

@aral
Copy link

aral commented Sep 29, 2021

Hey all, just got back from three weeks away, seeing the parents and was wondering how things are going on this. Is there anything you need from me to move this forward? Hope you’re all well.

<3

@cassidyjames
Copy link
Contributor

@aral I have been using and enjoying it. I think we just still need to figure out a shadow, mark it as ready for review, and get it reviewed. 😄

@davidmhewitt
Copy link
Member Author

And I had a poke at the accessibility stuff as promised but found it wasn't particularly consistent or reliable, plus it didn't behave like any other widget with the screen reader.

It seems like there should be a better way of doing it like adding the accessibility title to each "icon" and getting the keyboard focus to jump to each icon as you tab through, but this didn't seem to be working well either.

I guess I can find the "least bad" accessibility support and push that in here. I can also have a look at the shadow, but things got really busy with my day job recently.

@cassidyjames
Copy link
Contributor

cassidyjames commented Sep 29, 2021

@davidmhewitt I do think as long as we're matching what we have in master, it should be mergable. A11y should absolutely be addressed but since it sounds like it's not new to this branch, issues should be filed separately.

@cassidyjames
Copy link
Contributor

So I can add a shadow here e.g. with

add_effect (new ShadowEffect (100, 2));

However, I'm getting pretty bad artifacting and the border doesn't look right. Any pointers?

2021-10-26.13-30-40.mp4

@davidmhewitt
Copy link
Member Author

@cassidyjames Yeah, I tried doing this a couple of weeks ago too.

My first impression was that it feels like there's a double shadow sometimes. The very first time you open it after starting Gala, it seems kind of fine, then subsequent openings can get messy.

And based on this weird workaround that I never got to the bottom of, I'd imagine they're related:

// For some reason, on Odin, the height of the caption loses
// its padding after the first time the switcher displays. As a
// workaround, I store the initial value here once we have it.
float caption_height = -1.0f;

Probably needs some proper digging regarding the use and re-use of certain Clutter Actors and whether they're destroyed properly after they're finished with.

@danirabbit danirabbit marked this pull request as ready for review November 2, 2021 18:55
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

It seems like possibly the same issue we're running into with menu shadows, so I don't think we should hold this back over it. I'd like to get this merged since it's a long-standing issue and unblocks Wayland and we can iterate more in future branches if need be. Better to get it in at the beginning of the month where there's lots of time to test

@danirabbit danirabbit merged commit 52cbc3c into master Nov 2, 2021
@danirabbit danirabbit deleted the catts-in-tree branch November 2, 2021 18:56
@aral
Copy link

aral commented Nov 8, 2021

@davidmhewitt I do think as long as we're matching what we have in master, it should be mergable. A11y should absolutely be addressed but since it sounds like it's not new to this branch, issues should be filed separately.

Opened an issue to track accessibility of the widget here: #1301

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Waiting for input from the UX team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants