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

feat(Workspace): add missing settings and update design #557

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

acolombier
Copy link
Contributor

@acolombier acolombier commented Sep 4, 2024

This work is mostly focused on the look-n-feel.

Design

image

Current look

Screencast.from.04-09-24.16.29.31.mp4

Note there is currently a limitation impacting SVG rendering in iced 0.12.x, but this appear to be fixed in 0.13

Depends of:

@acolombier acolombier force-pushed the feat/workspaces-settings branch from 1ff8aec to 1c9e8cb Compare September 4, 2024 16:06
@acolombier acolombier force-pushed the feat/workspaces-settings branch 2 times, most recently from cbed42c to a4026b3 Compare September 4, 2024 16:42
@acolombier acolombier marked this pull request as ready for review September 4, 2024 16:42
@leviport leviport requested a review from a team September 4, 2024 16:43
@maria-komarova
Copy link

maria-komarova commented Sep 4, 2024

On the video it looks like the illustrations for workspaces spanning displays and being separate is very dark. Do you know why that it? Are those just SVGs?

I also can't tell from the video is workspace thumbnails change position on the illustration depending on their placement (left, right etc.)

Another question is if the illustrations change for light mode?

The rest of it looks nice on the video, thanks for working on it.

@acolombier
Copy link
Contributor Author

On the video it looks like the illustrations for workspaces spanning displays and being separate is very dark. Do you know why that it? Are those just SVGs?

Yes, this is due to the bug in iced I attached above.

I also can't tell from the video is workspace thumbnails change position on the illustration depending on their placement (left, right etc.)

Does that mean that, for example

  • if Left is selected, you get:

image

if Right is selected, you get:

image

If so, would it be worth adding the assets in the design?

Another question is if the illustrations change for light mode?

It does! :)

@git-f0x
Copy link
Contributor

git-f0x commented Sep 4, 2024

Should the trackpad gestures dynamically update depending on workspace orientation and thumbnail placement (once those gestures are implemented/configurable in cosmic-comp)?
E.g. if workspaces are horizontal and on the bottom, a four-finger swipe up opens the workspace overview, but if they're on the top, a swipe down would open the overview (and similar for vertical workspaces).

@acolombier
Copy link
Contributor Author

Should the trackpad gestures dynamically update depending on workspace orientation and thumbnail placement (once those gestures are implemented/configurable in cosmic-comp)?

This is already the case. Are you suggesting it shouldn't?

@git-f0x
Copy link
Contributor

git-f0x commented Sep 4, 2024

Oh, right, in the designs the text doesn't change, but in the video it works properly. (should they maybe be called Touchpad gestures, since that's how it's referred to on other pages?)
Though does it possibly make more sense to reverse the gesture direction (e.g. when workspaces are on the left, a swipe right opens them)?

@maria-komarova
Copy link

maria-komarova commented Sep 4, 2024

If so, would it be worth adding the assets in the design?

Assets are easy to add.
Does the thumbnail placement actually change in the overview? If not, we should wait until it does before introducing the setting.

Same thing with trackpad gestures. Do they actually change? They should be but I want to double check.

And same thing with workspace overview thumbnails options like Show workspace number and Show workspace name. I don't believe those are implemented yet so they shouldn't be present in Settings. The mockups you are looking at have been adjusted for first release with a lot of things removed from it.

About the bug in Iced - we should wait merging those illustrations until the bug is fixed.

@acolombier acolombier changed the title feat(Workspace): add missing settings and update design [DOT NOT MERGE] feat(Workspace): add missing settings and update design Sep 4, 2024
@acolombier acolombier changed the title [DOT NOT MERGE] feat(Workspace): add missing settings and update design [DO NOT MERGE] feat(Workspace): add missing settings and update design Sep 4, 2024
@acolombier
Copy link
Contributor Author

Assets are easy to add.

Great. Please ping me when they are in so I can update the PR!

Does the thumbnail placement actually change in the overview? If not, we should wait until it does before introducing the setting.

Same thing with trackpad gestures. Do they actually change?

Currently not - the setting is not applied yet. I don't quite know how it needs to be persisted for the cosmic-comp to pick it, assuming cosmic-comp already support it.

Note I'm not developing live on COSMIC as well, so this work was mostly focused on the UI only. I might give it a go at some point using a VM (sadly I cannot run cosmic on my setup due to a long standing issue)

The mockups you are looking at have been adjusted for first release with a lot of things removed from it.

Yes, most of the work I've done here is for the second release. AFAIK, Workspace as it stands is is already ready for the first release.

About the bug in Iced - we should wait merging those illustrations until the bug is fixed.

Agreed - this is already in the PR description

image

I have updated the PR title so it is more clear.

@acolombier acolombier force-pushed the feat/workspaces-settings branch from 87e4f3a to 43aa41e Compare October 20, 2024 12:46
@acolombier acolombier force-pushed the feat/workspaces-settings branch 2 times, most recently from 0227032 to 5ba0294 Compare November 7, 2024 21:38
@acolombier acolombier changed the title [DO NOT MERGE] feat(Workspace): add missing settings and update design feat(Workspace): add missing settings and update design Nov 7, 2024
@acolombier acolombier force-pushed the feat/workspaces-settings branch from 5ba0294 to 05e38a1 Compare November 7, 2024 21:43
@acolombier
Copy link
Contributor Author

Since libcosmic is now based on iced 0.13, the SVG bug has been resolved.

I have also added the extra assets for right and bottom workspace overview thumbnails as @maria-komarova requested.

Screencast.from.07-11-24.21.41.55.webm

@git-f0x
Copy link
Contributor

git-f0x commented Nov 7, 2024

Please remove or comment out non-functional settings, like the Workspace Overview Thumbnails section and Thumbnail placement. The App Library gesture also isn't implemented yet afaik, so that should also probably be commented out.
Also, the Trackpad gestures item should only show up if the system has a touchpad. See:

/// Uses `udev` to check if a touchpad device exists on the system.
fn system_has_touchpad() -> bool {
let Ok(mut enumerator) = udev::Enumerator::new() else {
return false;
};
let _res = enumerator.match_subsystem("input");
let Ok(mut devices) = enumerator.scan_devices() else {
return false;
};
devices.any(|device| {
device
.property_value("ID_INPUT_TOUCHPAD")
.map_or(false, |value| value == "1")
})
}

@git-f0x
Copy link
Contributor

git-f0x commented Nov 8, 2024

You disabled the Multi-monitor Behavior section, instead of the Workspace Overview Thumbnails (workspace_overview()) section.
The Workspaces Orientation section will have to be refactored to only show the left and top workspace orientation images (depending on selection), and not use the page.comp_workspace_config.workspace_thumbnail_placement variable, since it doesn't exist in cosmic-comp and cosmic-workspaces-epoch don't support bottom and right positions yet.

@acolombier
Copy link
Contributor Author

Thanks for the quick review @git-f0x - I have commented out features unsupported by the cosmic-workspaces-epoch. I'm going to try to add the orientation in there, the multi-monitor behaviour is probably something I won't be able to do as I'm still struggling to understand the cosmic-comp code. Is that something you could give me pointers to or do you reckon it's best letting someone else look into it?

@acolombier
Copy link
Contributor Author

Workspace Overview Thumbnails is supported and functional in cosmic-workspaces-epoch AFAICT - Multi-monitor Behavior isn't

The Workspaces Orientation section will have to be refactored to only show the left and top workspace orientation

That should already be the case (I've hidden the dropdown for left/right, top/bottom selection)

@git-f0x
Copy link
Contributor

git-f0x commented Nov 8, 2024

Workspace Overview Thumbnails is supported and functional in cosmic-workspaces-epoch AFAICT

Those settings were there previously, but were removed because they were non-functional. Toggling either doesn't change anything in workspaces.

Multi-monitor Behavior isn't

It's supported, and is present in the current master.

That should already be the case (I've hidden the dropdown for left/right, top/bottom selection)

The images cannot show up, since the chosen image depends on a non-existing variable (page.comp_workspace_config.workspace_thumbnail_placement), and that variable has to be removed for Settings to compile.

@acolombier
Copy link
Contributor Author

acolombier commented Nov 8, 2024

Workspace Overview Thumbnails is supported and functional in cosmic-workspaces-epoch AFAICT

Those settings were there previously, but were removed because they were non-functional. Toggling either doesn't change anything in workspaces.

Weird - this is working for me. Changing horizontal/vertical does change where the workspace thumbnails get displayed

Multi-monitor Behavior isn't

It's supported, and is present in the current master.

Ah great. I will bring it back then.

That should already be the case (I've hidden the dropdown for left/right, top/bottom selection)

The images cannot show up, since the chosen image depends on a non-existing variable (page.comp_workspace_config.workspace_thumbnail_placement), and that variable has to be removed for Settings to compile.

This PR depends of this one which does add the relevant settings. I've just implemented the support for right/bottom placement on cosmic-workspaces-epoch and testing right now. Should submit a PR in the next few hours

(I'm using a cargo.toml patch locally, which make the repo to point at my comp PR)

@git-f0x
Copy link
Contributor

git-f0x commented Nov 8, 2024

Weird - this is working for me. Changing horizontal/vertical does change where the workspace thumbnails get displayed

I'm referring to the Workspace Overview Thumbnails section, with show name and show number, not the Workspaces Orientation section for choosing vertical/horizontal workspaces.

This PR depends of pop-os/cosmic-comp#811 which does add the relevant settings. I've just implemented the support for right/bottom placement on cosmic-workspaces-epoch and testing right now. Should submit a PR in the next few hours

Oh, didn't know you had implemented it in workspaces as well. Great work!

@acolombier
Copy link
Contributor Author

with show name and show number, not the Workspaces Orientation section

Ah sorry, it's late over there. I've noticed that cosmic-workspaces-epoch does display the workspace number under the thumbnail, so I'll see if I can at least implement number toggle.

image

@acolombier acolombier force-pushed the feat/workspaces-settings branch from 64a07a7 to 53e4549 Compare November 8, 2024 10:24
@acolombier
Copy link
Contributor Author

I have implemented the features in cosmic-workspaces-epoch so I have restored the settings. I added the cosmic-workspaces-epoch as a dependency above, although the order release doesn't matter, only cosmic-comp setting definition needs to go first

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.

3 participants