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 definition for thumbnail placements #811

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

Conversation

acolombier
Copy link

@acolombier acolombier commented Sep 4, 2024

Add definition for thumbnail placement in workspace

Used in pop-os/cosmic-settings#557 and pop-os/cosmic-workspaces-epoch#104

@ryanabx
Copy link
Contributor

ryanabx commented Nov 10, 2024

I feel like workspace cosmetic info that the compositor doesn't need should be in a separate config, perhaps in the cosmic-workspaces-epoch repo. Since cosmic-comp needs workspace orientation and the per-display settings, it makes sense for them to be in cosmic-comp-config

The overall changes are really great though! Thank you for working on this.

@acolombier
Copy link
Author

I'm wondering if it would be even worth merging WorkspaceLayout and WorkspaceThumbnailPlacement together since the first on is kinda redundant. I feel like having the workspace setting definition in a single place is valuable tho, but agreed it should perhaps be moved out of cosmic-comp repo.
I think there is a risk that at some point, cosmic-comp needs information about workspace thumbnail placement to adapt certain behaviour so it would make a point on keeping config in a single crate

@ryanabx
Copy link
Contributor

ryanabx commented Nov 10, 2024

cosmic-comp needs information about workspace thumbnail placement to adapt certain behaviour

Sorry, I may not be seeing what you're seeing, but why would cosmic-comp need that?

@acolombier
Copy link
Author

There is no such a behaviour yet! It was more a matter of, this could be needed in the future.

@ryanabx
Copy link
Contributor

ryanabx commented Nov 11, 2024

Yeah, I suppose it's not too harmful to have it in cosmic-comp's config. The reason I contended on that is because cosmic-comp is used in projects that don't utilize the entire cosmic-session, so having a config value that's only used in cosmic-workspaces-epoch is clutter, when the value could be in cosmic-workspaces-epoch instead. The workspace orientation is something comp actually uses, thus it makes sense to have it there, and have cosmic-workspaces-epoch read the value.

@ryanabx
Copy link
Contributor

ryanabx commented Nov 11, 2024

@Drakulix what do you think about the above conversation?

@git-f0x
Copy link
Contributor

git-f0x commented Nov 12, 2024

One benefit of comp knowing the placement of workspace thumbnails is for tweaking the gestures required for opening the overview.
E.g. if workspaces are on the left, then a swipe right would open them, and if they're on the bottom, a swipe up would open them, etc.
Though this could possibly also be done with a workspaces config file, and comp reading from that, without adding enums to comp, and using defaults if the file doesn't exist/is malformed.

@acolombier
Copy link
Author

Does it makes sense for the WorkspaceLayout to be Vertical and WorkspaceThumbnailPlacement to be Left or Right? Looking at the original design in Figma, the answer seems to be now, but appreciate that this could be an oversight during design. If there is a consensus around no tho, I guess it could make sense to merge WorkspaceLayout and WorkspaceThumbnailPlacement as one enum, rather than two independent, which may be in conflict state (eg Vertical vs Left/Right)

@acolombier
Copy link
Author

Friendly ping @Drakulix in case you missed the notification :)

@Drakulix
Copy link
Member

I have not yet reviewed this, as I am still waiting @pop-os/ux to decide, if we want this feature.

@acolombier
Copy link
Author

This was developed following the Figma design as per mentioned here. Feedback about the UI was also provided and addressed.

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.

4 participants