Skip to content

FF: remove the use of currentlyRecording in EyetrackerControl#6567

Merged
TEParsons merged 9 commits intopsychopy:devfrom
mh105:dev-etRecord-comp
Jun 22, 2024
Merged

FF: remove the use of currentlyRecording in EyetrackerControl#6567
TEParsons merged 9 commits intopsychopy:devfrom
mh105:dev-etRecord-comp

Conversation

@mh105
Copy link
Contributor

@mh105 mh105 commented Jun 13, 2024

As @TEParsons might have guessed, I tried to use the EyetrackerControl component, and it still isn't working the way we want after #6463 and #6468. I just made a BF and was about to PR to release, then I realized you had a BF commit 9157901 to the dev branch already, which made essentially the same changes...

I guess great minds think alike :) And I like how you explicitly define the start() and stop() methods as opposed to using @status.setter. This is the same problem as we discussed for sound component start/stop and how it interacts with component .status.

I'm making this PR to add two additional things after comparing my BF with your BF commit 9157901:

  1. There is no need for the writeRoutineEndCode() for this EyetrackerControl component to toggle status to FINISHED. If it isn't FINISHED, the routine couldn't end anyways since continueRoutine will get turned back to True due to the EyetrackerControl component not being FINISHED.
  2. I find the use of currentlyRecording class attribute unnecessary and potentially problematic. For example, if I manually turned the eyetracker recording status to False without using a Stop Only EyetrackerControl component, then a Start Only EyetrackerControl component won't clear events properly because it relies on EyetrackerControl.currentlyRecording getting turned back to False. Couldn't we simply use self.tracker.isRecordingEnabled() to assess whether the eyetracker is recording or not?

@TEParsons
Copy link
Contributor

If it isn't FINISHED, the routine couldn't end anyways

Routines can end when not all Components are FINISHED - if the Routine is force ended (be it by Routine Settings, a Keyboard/Mouse/etc. Component or by a Code component), but I think removing this is fine anyway as we no longer need .status to be FINISHED now that .status isn't tied to starting and stopping recording.

I think you're right that .currentlyRecording isn't needed given that we have isRecordingEnabled, the only concern would be users who are using .currentlyRecording for their own custom code. Maybe add a @property to EyeTrackerControl which just returns self.tracker.isRecordingEnabled()?

Also where you see E128 it's fine to just do the indentation, you don't need to flag it and wait :)

@mh105
Copy link
Contributor Author

mh105 commented Jun 15, 2024

Sure. I just think we don't toggle the .status of any other component at the end of routine even when things are force ended, so it doesn't make sense to make an exception for EyetrackerControl. I added a Stop with Routine option for EyetrackerControl and wrote a corresponding writeRoutineEndCode() to write the code to stop.

I think this should be an option if the actionType contains Stop, because presumably on some routines that have mechanisms for forced ending with behavioral inputs may also want to end eye tracking along with it.

Re: .currentlyRecording, sure, I added a @property for backwards compatibility.

@TEParsons
Copy link
Contributor

@mh105 I think tests are failing because of a bug that's fixed in upstream - could you rebase/merge dev into this branch and push out again?

@mh105
Copy link
Contributor Author

mh105 commented Jun 22, 2024

@TEParsons All good now! Although see my comment in #6599, is there a bug that is somehow halting the experiment?

@TEParsons TEParsons merged commit d38eab6 into psychopy:dev Jun 22, 2024
@mh105 mh105 deleted the dev-etRecord-comp branch June 24, 2024 00:25
@peircej peircej added the 🐞 bug Issue describes a bug (crash or error) or undefined behavior. label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐞 bug Issue describes a bug (crash or error) or undefined behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants