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

Copy & Paste Activity Directives #1565

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

ivydeliz
Copy link
Contributor

@ivydeliz ivydeliz commented Nov 26, 2024

Feature #1544

I removed the keyboard shortcuts for Copy & Paste because I tried a few times, there were still open question and we didn't like the options, I'll try on a separate branch off of this one to see if i can make it work.

@ivydeliz ivydeliz changed the title Copy & Paste of Activity Directives Copy & Paste Activity Directives Nov 27, 2024
src/components/activity/ActivityDirectivesTable.svelte Outdated Show resolved Hide resolved
src/components/activity/ActivityDirectivesTable.svelte Outdated Show resolved Hide resolved
src/utilities/activities.ts Outdated Show resolved Hide resolved
src/utilities/clipboard.ts Outdated Show resolved Hide resolved
src/utilities/gql.ts Outdated Show resolved Hide resolved
src/utilities/gql.ts Outdated Show resolved Hide resolved
@ivydeliz ivydeliz marked this pull request as ready for review December 5, 2024 20:42
@ivydeliz ivydeliz requested a review from a team as a code owner December 5, 2024 20:42
src/utilities/effects.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@AaronPlave AaronPlave left a comment

Choose a reason for hiding this comment

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

Looking great, so nice to be able to copy paste! Left a bunch of comments but nothing huge.

@dandelany
Copy link
Collaborator

dandelany commented Dec 6, 2024

@ivydeliz gave a walkthrough this morning and it's looking good! A few issues/changes we discussed (some of these are duplicates of comments above):

  • bug with offsets when you paste two directives where one is anchored to the other - the offset time changes for the anchored directive, but it should remain the same offset, just anchored to the new directive
  • bug with keyboard shortcuts - the listener runs for all copy/paste keyboard actions even when not intended. Ideally should be possible to Ctrl+C most text on the page and Ctrl+V into any input box without triggering directive copy/paste
  • We decided to change the paste text in the menu when you right click on the timeline from "Paste at Time" to "Paste Directive at Time" when a single directive is copied, or eg. "Paste 5 Directives at Time" when multiple are copied

We also had a question about how to handle the case where some of the pasted activities fall outside of the plan bounds entirely. We currently allow this to happen - which is probably correct - but would be good to get input from @mattdailis or @Mythicaeda . We also noted a UI inconsistency - it seems that activity directives which fall before the plan bounds are shown with a "outside plan bounds" icon/warning in the directives table, but directives which are after the plan bounds don't show this - not clear if this is intentional or something we should fix in a follow-up

@Mythicaeda
Copy link
Contributor

Mythicaeda commented Dec 9, 2024

We also noted a UI inconsistency - it seems that activity directives which fall before the plan bounds are shown with a "outside plan bounds" icon/warning in the directives table, but directives which are after the plan bounds don't show this - not clear if this is intentional or something we should fix in a follow-up

I can shed some light on this, @dandelany. This inconsistency is because while we know for a fact whether or not an activity falls before the plan start time, we may not know if an activity falls after the plan end time due to anchors.

Quick Case breakdown:

  1. Activity A's start time is one hour after the plan's end time.
  2. Activity A's start time is one hour before the plan's end time. Activity B is anchored to start as soon as Activity A finishes. Activity A may run either two minutes or two hours.

In Case 1, we know statically that Activity A is outside the plan bounds. In Case 2, we don't know until the user simulates if Activity B will be within the plan bounds. We felt it would be more confusing (and possibly misleading) to the user to mark Case 1 and not Case 2 than it would be to not mark either case.

Changing this would require a DB change, as that's what runs the validation logic for activity start times.

@Mythicaeda
Copy link
Contributor

We also had a question about how to handle the case where some of the pasted activities fall outside of the plan bounds entirely. We currently allow this to happen - which is probably correct

My take is:

  • when copying an anchor chain, copy-paste should not change anchor relationships -- if the user copied an anchor chain and an activity that is not the root lands outside the plan bounds, we should let the user handle that
  • when you copy an activity that is anchored to something and paste it such that it is no longer anchored, then it's start offset should be at minimum plan-start and at maximum plan end (I'm assuming that if you're hovering over a time in the timeline that it's start offset is set to that time)
    • I think this logic still makes sense if it's also applied to activities that weren't anchored to anything when you copied them, but am willing to hear @mattdailis's thoughts if he disagrees

I feel the strongest about the first bullet.

@ivydeliz ivydeliz force-pushed the feature/copy-and-paste-of-activity-directives branch from b7cc98a to c045689 Compare December 19, 2024 03:34
@ivydeliz ivydeliz force-pushed the feature/copy-and-paste-of-activity-directives branch from c045689 to d46bd1c Compare December 19, 2024 03:40
@ivydeliz ivydeliz force-pushed the feature/copy-and-paste-of-activity-directives branch from d46bd1c to fece7d7 Compare December 19, 2024 03:48
@ivydeliz ivydeliz force-pushed the feature/copy-and-paste-of-activity-directives branch from fece7d7 to 2b27699 Compare December 19, 2024 03:57
@ivydeliz ivydeliz requested a review from AaronPlave December 19, 2024 04:08
@ivydeliz ivydeliz force-pushed the feature/copy-and-paste-of-activity-directives branch from 2b27699 to 943f915 Compare January 15, 2025 19:17
@ivydeliz ivydeliz requested a review from AaronPlave January 15, 2025 19:43
@ivydeliz
Copy link
Contributor Author

I removed the keyboard shortcuts for Copy & Paste because I tried a few times, there were still open question and we didn't like the options, I'll try on a separate branch off of this one to see if i can make it work.

Copy link
Contributor

@AaronPlave AaronPlave left a comment

Choose a reason for hiding this comment

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

One small comment but otherwise LGTM!

@ivydeliz ivydeliz force-pushed the feature/copy-and-paste-of-activity-directives branch from 943f915 to 53f70c2 Compare January 17, 2025 18:42
@ivydeliz ivydeliz force-pushed the feature/copy-and-paste-of-activity-directives branch from 79f06e2 to 00193a6 Compare January 21, 2025 20:23
@ivydeliz ivydeliz merged commit 7af9177 into develop Jan 21, 2025
5 checks passed
@ivydeliz ivydeliz deleted the feature/copy-and-paste-of-activity-directives branch January 21, 2025 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants