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

[WIP] u3d/installation: add feature to create shortcuts #336

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

niezbop
Copy link
Member

@niezbop niezbop commented Dec 5, 2018

Pull Request Checklist

  • My pull request has been rebased on master
  • I ran bundle exec rspec to make sure that my PR didn't break any test
  • I ran bundle exec rubocop to make sure that my PR is inline with our code style
  • I have read the code of conduct

Pull Request Description

This brings a small QoL improvement which enables one to create shortcuts for his Unity installs in any directory.

Relies on the win32-shortcut gem.

@niezbop niezbop force-pushed the installation/create_shortcut branch from da171f8 to ed3dce4 Compare December 5, 2018 15:10
@lacostej
Copy link
Member

lacostej commented Jan 8, 2019

Some questions

  • How does this windows specific dependency affects other platforms? is this tested on non Windows?
  • We could envision supporting other platforms, in which case we would have the same feature on Mac / Linux. In that case I would feel that we could move the platform specific dependencies in a class specific to the shortcut creation maybe.
  • tests!

More comments in the review itself.

@@ -30,6 +30,7 @@ Gem::Specification.new do |spec|
spec.add_dependency 'inifile', '>= 3.0.0', '< 4.0.0' # Parses INI files
spec.add_dependency 'plist', '>= 3.1.0', '< 4.0.0' # Generate the Xcode config plist file
spec.add_dependency 'security', '= 0.1.3' # macOS Keychain manager, a dead project, no updates expected
spec.add_dependency 'win32-shortcut', '>= 0.3.0'
Copy link
Member

Choose a reason for hiding this comment

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

see overall comment related to platform specific dependency.


if File.exist? lnk_path
UI.message "Shortcut already exists at #{lnk_path}"
return Win32::Shortcut.open(lnk_path)
Copy link
Member

Choose a reason for hiding this comment

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

why do we open it here?

c.syntax = 'u3d shortcuts [-u | --unity_version <version>] [-t | --target <folder>]'
c.option '-u', '--unity_version STRING', String, 'Creates a shortcut for this version of Unity only.'
c.option '-d', '--directory STRING', String, 'Specifies which directory to create the shortcuts in'
c.example 'Create a shortcut for all installed versions on the Desktop', 'u3d shortcuts -d ~/Desktop'
Copy link
Member

Choose a reason for hiding this comment

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

is it risky to create shortcuts for all versions without specifying --all?

end

shortcut = Win32::Shortcut.new(lnk_path) do |s|
s.description = "Shortcut to Unity.exe for Unity #{unity_version}. Automatically created by u3d."
Copy link
Member

Choose a reason for hiding this comment

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

do we need to remove shortcuts when uninstalling?

command :shortcuts do |c|
c.syntax = 'u3d shortcuts [-u | --unity_version <version>] [-t | --target <folder>]'
c.option '-u', '--unity_version STRING', String, 'Creates a shortcut for this version of Unity only.'
c.option '-d', '--directory STRING', String, 'Specifies which directory to create the shortcuts in'
Copy link
Member

Choose a reason for hiding this comment

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

This seems optional. It should document where it creates the screenshot when it isn't passed.

require 'win32-shortcut'
full_exe_path = File.expand_path(path_to_unity_exe)
lnk_parent = if target_directory.nil?
File.expand_path('..', full_exe_path)
Copy link
Member

Choose a reason for hiding this comment

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

weird implementation of a default no?

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be the desktop? In which case, should it be passed to the function (and let the caller define it).

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

Successfully merging this pull request may close these issues.

2 participants