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

Workspace switcher #361

Open
wants to merge 58 commits into
base: main
Choose a base branch
from
Open

Workspace switcher #361

wants to merge 58 commits into from

Conversation

lenemter
Copy link
Member

@lenemter lenemter commented Feb 12, 2025

Screenshot from 2025-02-13 11 28 29
Screenshot from 2025-02-13 11 27 28

Fixes #335
Fixes #224
Closes #347

Requires elementary/gala#2265

There are some things that are not ideal, i.e. only four windows are shown, no separator between apps and the workspace switcher but I'll leave them for later because this PR is already too big.

@lenemter lenemter added the Needs Design Waiting for input from the UX team label Feb 12, 2025
@lenemter
Copy link
Member Author

lenemter commented Feb 13, 2025

This is mostly ready functionally-wise. I would appreciate if anyone tested this for any bugs/crashes.

This has poor styling and I'm bad at CSS, so I'll be glad if @danirabbit helped me with this. Also icon groups lack some animations and I have no experience working with Libadwaita animations either, so I would appreciate if someone took this over.

@lenemter lenemter marked this pull request as ready for review February 13, 2025 18:42
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.

This is dope. Definitely gonna have to use it regularly in practice to see how this changes things. But I like it a lot already.

Biggest problem is it crashes whenever there's an open file portal

One thing I'm thinking right away is that it's redundant with the adding non-pinned launchers to the dock. It seems like we don't need to do that anymore since they're already added to the workspace switcher. Thoughts?

I think the "new workspace" button should remain static always at the end instead of fading in a new one. This feels more obvious with my styles branch that it kinda feels like the button is running away or something.

@alainm23

This comment was marked as resolved.

@lenemter
Copy link
Member Author

This is ready for review now

@teamcons
Copy link

teamcons commented Feb 16, 2025

Installed here. Running it on metal, and will test it for a while. Looks pretty solid

some papercuts:
-Order of apps in workspace preview is different in the launcher from the one in multitasking.
-closing all windows on a workspace B between two other workspaces A and C, the switcher displays the animation of the preview of the workspace C on the far right being removed instead of the empty B one. (There will still be a preview for C being shown)
-some app have at first the purple star icon in switcher workspace preview. Closing them then reopening fixes this

@lenemter
Copy link
Member Author

-Order of apps in workspace preview is different in the launcher from the one in multitasking.

I would prefer to implement window sorting in a follow up branch

-closing all windows on a workspace B between two other workspaces A and C, the switcher displays the animation of the preview of the workspace C on the far right being removed instead of the empty B one. (There will still be a preview for C being shown)

Fixed!

-some app have at first the purple star icon in switcher workspace preview. Closing them then reopening fixes this

Can you give me an example of such app? I cannot reproduce the issue on my side :(

@teamcons
Copy link

-Order of apps in workspace preview is different in the launcher from the one in multitasking.

I would prefer to implement window sorting in a follow up branch

Yeah fair. Youve already done a lot in a short time too.

Can you give me an example of such app? I cannot reproduce the issue on my side :(

I cannot reproduce it consistently, it happened on "old notejot" as a startup app, upon opening the session
Id rather have this left aside waiting whether other people had it or more is known

@danirabbit

This comment was marked as resolved.

@lenemter
Copy link
Member Author

lenemter commented Feb 17, 2025

@danirabbit sorry, I didn't test well enough. Should be fixed now

@lenemter lenemter mentioned this pull request Feb 17, 2025
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.

I think this looks good to me to get into main and do some more thorough testing etc :)

Great job! 🚀

@danirabbit danirabbit removed the Needs Design Waiting for input from the UX team label Feb 18, 2025
Copy link
Member

@leolost2605 leolost2605 left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay, exams for this semester have started so I'm kinda limited on time right now :/
This looks pretty good, i just added some minor comments. Another thing: Have you tested for memory leaks? I think I remember bindings being a problem sometimes not sure whether it's the case here though.
I'll have some time in a day or two so I'll do a proper review then and check for mem leaks, etc. :)

@lenemter
Copy link
Member Author

lenemter commented Feb 24, 2025

@leolost2605 I didn't find any memory leaks, every destructor of App, Laucncher, Window, IconGroup and Workspace is called. There are g_signal_connect_data though but it is used in the same spots as in main

@lenemter
Copy link
Member Author

lenemter commented Feb 24, 2025

I noticed there are many bind_property... I guess we don't need to do unbind?

Here's the source code of constructor of GLib.Binding

static void
g_binding_constructed (GObject *gobject)
{
  GBinding *binding = G_BINDING (gobject);
  GBindingTransformFunc transform_func = default_transform;
  GObject *source, *target;
  GQuark source_property_detail;
  GClosure *source_notify_closure;

  /* assert that we were constructed correctly */
  source = g_weak_ref_get (&binding->context->source);
  target = g_weak_ref_get (&binding->context->target);
  g_assert (source != NULL);
  g_assert (target != NULL);
  g_assert (binding->source_property != NULL);
  g_assert (binding->target_property != NULL);

  /* we assume a check was performed prior to construction - since
   * g_object_bind_property_full() does it; we cannot fail construction
   * anyway, so it would be hard for use to properly warn here
   */
  binding->source_pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (source), binding->source_property);
  binding->target_pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (target), binding->target_property);
  g_assert (binding->source_pspec != NULL);
  g_assert (binding->target_pspec != NULL);

  /* switch to the invert boolean transform if needed */
  if (binding->flags & G_BINDING_INVERT_BOOLEAN)
    transform_func = default_invert_boolean_transform;

  /* set the default transformation functions here */
  binding->transform_func = transform_func_new (transform_func, transform_func, NULL, NULL);

  source_property_detail = g_quark_from_string (binding->source_property);
  source_notify_closure = g_cclosure_new (G_CALLBACK (on_source_notify),
                                          binding_context_ref (binding->context),
                                          (GClosureNotify) binding_context_unref);
  binding->source_notify = g_signal_connect_closure_by_id (source,
                                                           gobject_notify_signal_id,
                                                           source_property_detail,
                                                           source_notify_closure,
                                                           FALSE);

  g_object_weak_ref (source, weak_unbind, binding_context_ref (binding->context));

  if (binding->flags & G_BINDING_BIDIRECTIONAL)
    {
      GQuark target_property_detail;
      GClosure *target_notify_closure;

      target_property_detail = g_quark_from_string (binding->target_property);
      target_notify_closure = g_cclosure_new (G_CALLBACK (on_target_notify),
                                              binding_context_ref (binding->context),
                                              (GClosureNotify) binding_context_unref);
      binding->target_notify = g_signal_connect_closure_by_id (target,
                                                               gobject_notify_signal_id,
                                                               target_property_detail,
                                                               target_notify_closure,
                                                               FALSE);
    }

  if (target != source)
    {
      g_object_weak_ref (target, weak_unbind, binding_context_ref (binding->context));

      /* Need to remember separately if a target weak notify was installed as
       * unlike for the source it can exist independently of the property
       * notification callback */
      binding->target_weak_notify_installed = TRUE;
    }

  g_object_unref (source);
  g_object_unref (target);
}

It creates strong references via source = g_weak_ref_get (&binding->context->source); and target = g_weak_ref_get (&binding->context->target);, then creates weak references via g_object_weak_ref and releases strong references. In theory there shouldn't be any memory leaks when using bind_property

Copy link
Member

@leolost2605 leolost2605 left a comment

Choose a reason for hiding this comment

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

There are some minor quirks (e.g. I get warnings WorkspaceSystem.vala:51: WorkspaceSystem.sync_windows: Unexpected window workspace index: 3 when moving windows between workspaces) but I'm with @danirabbit here lets goo :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Hide the dock when empty Add new workspace button to the dock Explicit workspace switching
5 participants