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

Add Control Mod to Track Group Selection #132

Open
wants to merge 2 commits into
base: reuse_timeline
Choose a base branch
from

Conversation

ALevansSamsung
Copy link
Collaborator

Track Groups did not previously act the same as Tracks when it came to selection. Control modifier should now preserve selection.

@cdamus
Copy link
Collaborator

cdamus commented Dec 2, 2024

I don't understand what this change is intended to accomplish. What am I as a reviewer supposed to test to verify the changes? I'm suspicious of the mention of Ctrl key in juxtaposition with the mouse pointer because on Mac platform this doesn't mean the same thing as on others: it summons the context menu.

@ALevansSamsung
Copy link
Collaborator Author

I don't understand what this change is intended to accomplish. What am I as a reviewer supposed to test to verify the changes? I'm suspicious of the mention of Ctrl key in juxtaposition with the mouse pointer because on Mac platform this doesn't mean the same thing as on others: it summons the context menu.

Currently when control clicking a track in the perfetto ui this changes the selected state of a track without clearing the previous state. This PR matches this functionality in the track groups as well. Track groups also removed the previous track selection state regardless of whether control was held.

@cdamus
Copy link
Collaborator

cdamus commented Dec 2, 2024

Currently when control clicking a track in the perfetto ui this changes the selected state of a track without clearing the previous state. This PR matches this functionality in the track groups as well. Track groups also removed the previous track selection state regardless of whether control was held.

I'm still not getting it. Track (group) selection is controlled by the little check boxes that appear when you drag out a selection rectangle over the tracks. What has this got to do with the Ctrl modifier? That isn't ordinarily used with check boxes, is it?

@ALevansSamsung
Copy link
Collaborator Author

When you click on track or track group label, the track header turns blue. This was meant to fix an issue where the user wanted to be able to click on a set of tracks to be able to combine them. What I did was add the ability to select tracks and use a plugin to access this information.

The PR on Sokatoa (https://github.com/android-graphics/sokatoa/pull/1470) would use this plugin point to be able to preselect the combination track dialog with this information. The Perfetto side of this was previous merged (#114), but the Sokatoa side of the PR got left in limbo. I started working on the Sokatoa side again and noticed an inconsistency in how the control modifier was working between track groups and tracks.

@cdamus
Copy link
Collaborator

cdamus commented Dec 2, 2024

BTW, if anything in Perfetto is counting on a mouse click event having the ctrlKey property set, then that won't work on Mac because this mouse event never happens. When clicking the mouse with Ctrl key down, the browser sends a contextmenu event to the DOM element, not any kind of mouse event.

On Mac platform probably what Perfetto should be looking for is the Command key to add to a selection. But I can't say for certain as I don't know what this selection scenario is for.

@cdamus
Copy link
Collaborator

cdamus commented Dec 2, 2024

When you click on track or track group label, the track header turns blue. This was meant to fix an issue where the user wanted to be able to click on a set of tracks to be able to combine them. What I did was add the ability to select tracks and use a plugin to access this information.

Ah, OK. Now I understand that this is all new selection behaviour in the Perfetto fork. It doesn't actually work as intended on Mac, so I cannot really test this.

@ALevansSamsung ALevansSamsung requested review from dfriederich and removed request for cdamus December 3, 2024 15:00
@ALevansSamsung
Copy link
Collaborator Author

It looks like tree-widget, which also handles multiselection, has a specific case for MacOs when it comes to checking the ctrl mask.

    /**
     * Determine if the tree modifier aware event has a `ctrlcmd` mask.
     * @param event the tree modifier aware event.
     *
     * @returns `true` if the tree modifier aware event contains the `ctrlcmd` mask.
     */
    protected hasCtrlCmdMask(event: TreeWidget.ModifierAwareEvent): boolean {
        return isOSX ? event.metaKey : event.ctrlKey;
    }

@cdamus
Copy link
Collaborator

cdamus commented Dec 3, 2024

It looks like tree-widget, which also handles multiselection, has a specific case for MacOs when it comes to checking the ctrl mask.

Indeed, if Perfetto is after a similar multiselection behaviour, then that is the way to do it: with the Command key. It's a bit frustrating, isn't it, that these frameworks don't have appropriate abstractions for these platform variation points? We shouldn't have to be concerned in mouse handlers with which modifier key is depressed ... alas.

@ALevansSamsung
Copy link
Collaborator Author

It looks like tree-widget, which also handles multiselection, has a specific case for MacOs when it comes to checking the ctrl mask.

Indeed, if Perfetto is after a similar multiselection behaviour, then that is the way to do it: with the Command key. It's a bit frustrating, isn't it, that these frameworks don't have appropriate abstractions for these platform variation points? We shouldn't have to be concerned in mouse handlers with which modifier key is depressed ... alas.

I hoped it would be like how touchpads are handled and abstracted. A touchpad "zoom in" finger motion is treated as a control + scroll up in html. I found this out accidentally when setting up the mouse zoom for the texture widget.

@ALevansSamsung
Copy link
Collaborator Author

@cdamus I added a change to support multiselecting tracks on MacOS. Can you test this as none of us use Mac?

@ALevansSamsung ALevansSamsung force-pushed the improve-track-selection-support branch from 4f1cab1 to fc9a426 Compare December 3, 2024 19:09
@cdamus
Copy link
Collaborator

cdamus commented Dec 3, 2024

Works great! Thanks @ALevansSamsung.

CleanShot 2024-12-03 at 15 03 36

Aside: meta key? I haven't seen one of those since my work on Sun SparcStations at Nortel in the 90s 😉

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.

2 participants