-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add convert to gif support #11
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
Conversation
… clipboard functionality - Consolidated iOS simulator and Android emulator handling into dedicated structs with common interface for screenshot and recording operations. - Enhanced screenshot and recording commands to support active device detection and improved error handling. - Introduced utility functions for file management, clipboard operations, and command execution. - Updated command flags for screenshot and recording commands to include options for copying file paths to clipboard and converting recordings to GIF format. - Added context handling for recording operations to allow graceful termination. - Updated dependencies in go.mod to include clipboard package for clipboard operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the iOS simulator and Android emulator management code with major changes to support new functionality including GIF conversion and clipboard operations. The changes include significant code reorganization, introduction of new dependencies, and addition of media conversion features.
- Refactors media capture logic to use an interface-based approach with iOS and Android capturers
- Adds clipboard support and GIF conversion functionality to media commands
- Extracts constants and utility functions into separate files for better organization
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Adds new dependencies for clipboard operations and table formatting |
| cmd/constants.go | Introduces centralized constants for commands, states, and file extensions |
| cmd/utils.go | New utility functions for file operations, clipboard, and video conversion |
| cmd/media.go | Complete refactor using interface pattern for device capture operations |
| cmd/list.go | Updates to use new constants and improved Android emulator detection |
| cmd/device.go | Updates function signatures and constant usage throughout device operations |
| func convertToGIF(inputFile, outputFile string) error { | ||
| if !commandExists(CmdFFmpeg) { | ||
| return fmt.Errorf("'%s' is not installed. Please install ffmpeg to use the GIF conversion feature", CmdFFmpeg) | ||
| } | ||
|
|
||
| fmt.Println("Converting to GIF...") | ||
| cmd := exec.Command(CmdFFmpeg, "-i", inputFile, "-vf", "fps=10,scale=480:-1:flags=lanczos", "-c", "gif", "-f", "gif", outputFile) |
Copilot
AI
Jul 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GIF conversion parameters are hardcoded. Consider making fps, scale, and other conversion parameters configurable through function parameters or constants to improve maintainability and allow users to customize output quality.
| func convertToGIF(inputFile, outputFile string) error { | |
| if !commandExists(CmdFFmpeg) { | |
| return fmt.Errorf("'%s' is not installed. Please install ffmpeg to use the GIF conversion feature", CmdFFmpeg) | |
| } | |
| fmt.Println("Converting to GIF...") | |
| cmd := exec.Command(CmdFFmpeg, "-i", inputFile, "-vf", "fps=10,scale=480:-1:flags=lanczos", "-c", "gif", "-f", "gif", outputFile) | |
| func convertToGIF(inputFile, outputFile string, fps int, scale string) error { | |
| if !commandExists(CmdFFmpeg) { | |
| return fmt.Errorf("'%s' is not installed. Please install ffmpeg to use the GIF conversion feature", CmdFFmpeg) | |
| } | |
| fmt.Println("Converting to GIF...") | |
| if fps <= 0 { | |
| fps = 10 // Default FPS | |
| } | |
| if scale == "" { | |
| scale = "480:-1" // Default scale | |
| } | |
| cmd := exec.Command(CmdFFmpeg, "-i", inputFile, "-vf", fmt.Sprintf("fps=%d,scale=%s:flags=lanczos", fps, scale), "-c", "gif", "-f", "gif", outputFile) |
| _, err := newIOSSimulator(args[0]) | ||
| isDevice := err == nil | ||
| if !isDevice { | ||
| _, err = newAndroidEmulator(args[0]) | ||
| isDevice = err == nil |
Copilot
AI
Jul 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code creates iOS and Android simulator/emulator objects just to check if the device exists, then discards them and creates new ones later. Consider extracting device validation into a separate function to avoid redundant object creation.
| _, err := newIOSSimulator(args[0]) | |
| isDevice := err == nil | |
| if !isDevice { | |
| _, err = newAndroidEmulator(args[0]) | |
| isDevice = err == nil | |
| isDevice, err := validateDevice(args[0]) | |
| if err != nil { | |
| return err |
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| var deviceID, outputFile string | ||
| if len(args) > 0 { | ||
| _, err := newIOSSimulator(args[0]) |
Copilot
AI
Jul 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate device validation logic with redundant object creation. This is the same performance issue as in the screenshot command.
cmd/list.go
Outdated
| } | ||
|
|
||
| _ = table.Append(device.Type, device.Name, device.State, udid, runtime) | ||
| table.Append([]string{device.Type, device.Name, device.State, udid, runtimeVal}) |
Copilot
AI
Jul 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table.Append method is being called with a slice argument, but the original code used individual string arguments. This change may cause a runtime error depending on the tablewriter library's API expectations.
| table.Append([]string{device.Type, device.Name, device.State, udid, runtimeVal}) | |
| table.Append(device.Type, device.Name, device.State, udid, runtimeVal) |
cmd/media.go
Outdated
| fullPath := ensureExtension(outputFile, ExtPNG) | ||
| devicePath := "/sdcard/screenshot.png" | ||
|
|
||
| defer e.runADB("shell", "rm", devicePath) |
Copilot
AI
Jul 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using defer for cleanup commands that can fail silently may mask important errors. Consider handling the cleanup error explicitly or logging when cleanup fails.
| defer e.runADB("shell", "rm", devicePath) | |
| defer func() { | |
| if err := e.runADB("shell", "rm", devicePath); err != nil { | |
| fmt.Printf("Warning: failed to clean up temporary file on device: %v\n", err) | |
| } | |
| }() |
| fmt.Printf("Android emulator '%s' is already running\n", deviceID) | ||
|
|
||
| // Save as last started device even if already running | ||
| udid, name := findRunningAndroidEmulator(deviceID) |
Copilot
AI
Jul 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The function findRunningAndroidEmulator is called to get both udid and name, but the existing logic above already has deviceID. Consider whether the name retrieval is necessary or if the function signature could be simplified.
39bc60a to
31a9080
Compare
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Fixes # (issue)
Type of change
Refactor most of logics
Add copy and convert to gif function
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Test Configuration:
Checklist:
Screenshots (if applicable):
Please add screenshots to help explain your changes.
Additional Notes:
Add any other notes about the pull request here.