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

adding transition #752

Closed
wants to merge 5 commits into from
Closed

adding transition #752

wants to merge 5 commits into from

Conversation

Arian8j2
Copy link

@Arian8j2 Arian8j2 commented Dec 24, 2021

Accept my apology

So this is my first ever PR and I'm not that much familiar with Picom and Compton code base yet and i really understand if this not going to get merged, But i said give it a try.

How works

This PR adds transitions to windows, It basically gives static offset from specific direction to window and then transition form there.

Configurations

I tried to replicate CSS transition properties in configs, So these are configs:

  • transition-duration
  • transition-offset
  • transition-direction
  • transition-timing-function
  • transition-rule (similar to opacity rule but it changes transition direction)

There are more details about each config in picom.sample.config file.

Demo

demo

@godalming123
Copy link

godalming123 commented Jan 14, 2022

Is it possible to make the transition direction depend on wether your going to the next/previos workspace instead of just what app is transitioning.

Sorry I see you can use

transition-direction = "smart-x";

or

transition-direction = "smart-x";

for this functionality.

Copy link

@godalming123 godalming123 left a comment

Choose a reason for hiding this comment

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

This looks great from the gif I havent looked at how clean the code is but so far so good! I would suggest one change though which is to update the manpage so it tells you about these new property's.

@Arian8j2
Copy link
Author

Arian8j2 commented Jan 14, 2022

@godalming123

Is it possible to make the transition direction depend on wether your going to the next/previos workspace instead of just what app is transitioning.

Sorry I see you can use

transition-direction = "smart-x";

or

transition-direction = "smart-x";

for this functionality.

I don't think any way exists to find out which workspace is currently on because there are lot's of different window manager out there. So basically smart-x and smart-y are not related to work spaces, they look for window width and if thats greater than 80% of screen width or window gets like whole screen it change direction of that window to left or up and so on...

This looks great from the gif I havent looked at how clean the code is but so far so good! I would suggest one change though which is to update the manpage so it tells you about these new property's.

Happy to hear you enjoyed the looks, I totally forgot about man pages but will look for it in the future, Thanks for mentioning that.

Copy link
Owner

@yshui yshui left a comment

Choose a reason for hiding this comment

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

Thanks for your work!

In general I have some reservation about the compositor moving users window around (changing opacity/color/etc is fine). The reason being the confusion this could cause, because although the window is shown at a certain location on screen, it doesn't accept user interaction at that location.

But I guess it is for the user to decide whether they want this. So I won't reject this PR on that ground.

This PR is somewhat messy, I would like to see some clean ups:

  • bug fixes shouldn't be in separate commits. each commit should not have known bugs at the time of merge.
  • changes should be splitted into smaller commits. this is to make it easier in the future to locate origin of bugs. as an example, for this PR, you could split it into:
    • add config options
    • add easing functions
    • implement transition
    • implement transition rules.

As a side note, I wonder if the current fade-in/fade-out feature can be implemented with the same transition mechanism, if you make it more generic.

@@ -1,3 +1,28 @@
################################
Copy link
Owner

Choose a reason for hiding this comment

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

There is not enough explanation here for:

  • what does transition do? i.e. what is being transitioned?
  • what is transition-offset?
  • what is the meaning of each transition-direction?

@@ -581,6 +582,10 @@ char *parse_config(options_t *opt, const char *config_file, bool *shadow_enable,

.track_leader = false,

.transition_direction = -1,
Copy link
Owner

Choose a reason for hiding this comment

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

Wrong indentation size. Can you run clang-format over your changes?


// Transition
if(w->transition_time >= 0.0f && w->transition_time <= 1.0f) {
// TODO: use refresh rate instead of using static 60 fps calculation
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to see this todo completed before merging

unsigned int direction = w->transition_direction;

// Smart direction
if(direction > TRANSITIONDIR_TOP) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't quite understand the logic here, maybe because it is not explained what the transition directions mean.

@@ -0,0 +1,173 @@
#include <math.h>
Copy link
Owner

Choose a reason for hiding this comment

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

This file doesn't follow picom's naming convention.

@Arian8j2
Copy link
Author

Moved to #766.

@Arian8j2 Arian8j2 closed this Jan 24, 2022
@Arian8j2 Arian8j2 deleted the next branch April 25, 2022 08:39
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