chore: Stripped dead code #4425

Semty · 2023-05-19T21:49:43Z

I have read CONTRIBUTING.md
This implements/fixes issue #.

Description:

Stripped dead code throughout the project.
It's a good practice to perform from time to time, leaving only the code that is actually in use.

Note:

I'm not sure whether this project still accepts pull requests from not core team members.
@low-batt, it seems to me that you're the most active participant lately, could you, please, help me with that?

low-batt · 2023-05-19T23:43:44Z

@Semty, you definitely do not need to be one of IINA's core developers to propose changes.

IINA is slow at reviewing and merging PRs as the senior developers are very busy. Usually it occurs in spurts when those developers have time to focus on IINA.

I am a newbie junior contributor. I don't merge, but I do review PRs. No promises, but I will try and find time to take a look at this one this weekend. This really needs a review by the senior developers as it covers areas of IINA I am unfamiliar with.

At the moment the senior developers are focused on getting a release out. The release branch as already been cut. The hold up is the need to patch some problems in the updated dependencies.

Commits going into develop now will be going into the following release. Which is why this is sort of a good time to clear out dead code. One concern is that there is resistance to refactoring/cleanup with so many PRs outstanding over the fear of introducing lots of merge conflicts.

While on the topic of contributing I should point out that although CONTRIBUTING indicates the latest version of Xcode must be used, at the moment we are building with Xcode 13 as well as Xcode 14 in order to support macOS 10.11 and 10.12, which Xcode 14 dropped.

Semty · 2023-05-20T10:51:56Z

@Semty, you definitely do not need to be one of IINA's core developers to propose changes.

IINA is slow at reviewing and merging PRs as the senior developers are very busy. Usually it occurs in spurts when those developers have time to focus on IINA.

I am a newbie junior contributor. I don't merge, but I do review PRs. No promises, but I will try and find time to take a look at this one this weekend. This really needs a review by the senior developers as it covers areas of IINA I am unfamiliar with.

At the moment the senior developers are focused on getting a release out. The release branch as already been cut. The hold up is the need to patch some problems in the updated dependencies.

Commits going into develop now will be going into the following release. Which is why this is sort of a good time to clear out dead code. One concern is that there is resistance to refactoring/cleanup with so many PRs outstanding over the fear of introducing lots of merge conflicts.

While on the topic of contributing I should point out that although CONTRIBUTING indicates the latest version of Xcode must be used, at the moment we are building with Xcode 13 as well as Xcode 14 in order to support macOS 10.11 and 10.12, which Xcode 14 dropped.

@low-batt hey! Got it, thank you so much for your thorough response 👌.
Let me know if you find anything that should not be removed, I'm open to fix it if needed

low-batt

I pulled the PR locally, built IINA and confirmed IINA started and was able to play a video.

I'm unfamiliar with a lot of this code, so the real review must be done by the senior developers. I had planned on not requesting changes and leaving it to those developers to request changes as I am unsure as to whether they would agree with some of my comments. However…

I'm certain this PR in its current state will be rejected due to including changes to generated sources. This is an existing pitfall for new contributors. I have entered issue #4429 for the failure to properly identify generated sources. See that issue and my inline comment.

On my other comments, feel free to wait and see what the other developers think.

Thank you for proposing these changes to IINA!

iina/MPVCommand.swift

low-batt · 2023-05-21T19:09:19Z

iina/MPVController.swift

@@ -1394,7 +1360,7 @@ class MPVController: NSObject {
  }
 }

-fileprivate func mpvGetOpenGLFunc(_ ctx: UnsafeMutableRawPointer?, _ name: UnsafePointer<Int8>?) -> UnsafeMutableRawPointer? {
+fileprivate func mpvGetOpenGLFunc(_: UnsafeMutableRawPointer?, _ name: UnsafePointer<Int8>?) -> UnsafeMutableRawPointer? {


Will have to see what the senior developers think, but I'm strongly against removing the name of parameters, even if they are not used. After this change a developer reading this code can't tell what it is that is being passed and ignored.

+1 for not changing

iina/MPVOption.swift

iina/MPVProperty.swift

iina/ShooterSubtitle.swift

low-batt · 2023-05-21T19:33:25Z

iina/VideoView.swift

+  _: CVDisplayLink, _: UnsafePointer<CVTimeStamp>,
+  _: UnsafePointer<CVTimeStamp>,
+  _: CVOptionFlags,
+  _: UnsafeMutablePointer<CVOptionFlags>,
  _ context: UnsafeMutableRawPointer?) -> CVReturn {


Although unused with respects to the code, these parameter names effectively provide documentation for developers reading the code. See what the senior developers say, but I am against removing parameter names.

Hmm, I tend to disagree here. My two points:

In my opinion, it actually better to omit some stuff that you don't need to perform your logic, you have less things to grasp and see only the important ones

If you want to change/debug this piece of code, it's better if you're familiar with it a bit more than just knowing the parameter names. It can be done really easily by checking where this function is used (CVDisplayLinkSetOutputCallback) and locating the parameter (CVDisplayLinkOutputCallback). After that you'll see the complete documentation with everything you may need

But, of course, I understand that it can be a hot topic, so I'm fully okay to return parameter names in every function even if it's not used

On this one lets see what the senior developers think.

I'd like to keep it as it is.

iina/ViewLayer.swift

saagarjha

I appreciate konmari efforts. Lots of this needs to go. Deciding which parts, though, is complicated. In general, these things should probably not be removed:

Sets of related properties or methods that exist for completeness: e.g. if we have setString, setInt, setDouble, … it doesn't make sense to remove one of these because it's not used. People expect the full set of APIs to be available.
Things that were just added and intend to be used in the near future. Determining this will probably be hard. cc @lhc70000 @uiryuu since they should sign off on this.

Marking for changes because some of these definitely need to be rolled back–see the comments for the ones I spotted.

saagarjha · 2023-05-23T04:11:09Z

iina/AppDelegate.swift

-/** Tags for "Open File/URL" menu item when "Always open file in new windows" is off. Vice versa. */
-fileprivate let NormalMenuItemTag = 0


As far as I can tell this was never used, perhaps worth adjusting the code to reflect that?

iina/Aspect.swift

saagarjha · 2023-05-23T04:14:10Z

iina/CollapseView.swift

@@ -55,7 +55,7 @@ class CollapseView: NSStackView {
    updateContentView(animated: animated)
  }

-  @objc private func triggerAction(_ sender: NSControl) {
+  @objc private func triggerAction() {


You can't remove this parameter, see https://developer.apple.com/library/archive/documentation/General/Conceptual/Devpedia-CocoaApp/TargetAction.html#//apple_ref/doc/uid/TP40009071-CH3-SW2

@saagarjha hey! thank you for your review!

You can't remove this parameter, see https://developer.apple.com/library/archive/documentation/General/Conceptual/Devpedia-CocoaApp/TargetAction.html#//apple_ref/doc/uid/TP40009071-CH3-SW2

are you sure? it works on iOS to omit the sender

It's legal on iOS, but not on macOS. Things will mostly still work because the argument register will probably not get touched but it's good to do it correctly.

iina/Extensions.swift

saagarjha · 2023-05-23T04:17:45Z

iina/InfoDictionary.swift

-  /// A Boolean value that indicates whether this executable was an optimized (not debug) build.
-  #if DEBUG
-  let isDebug = true
-  #else
-  let isDebug = false
-  #endif
-


Pretty sure this was just added recently, @low-batt you should probably look at this and see if you needed it

Yes, this was recently added as a part of issue #4383 / PR #4385. However my role in that issue was to just get it started and hand it off to @uiryuu. Will have to ask @uiryuu about this change.

Let keep this, although there isn't any code using this. We might want to use this in the future.

saagarjha · 2023-05-23T04:20:17Z

iina/JavascriptPlugin.swift

-
-    if externalURL != nil {
-      self.isExternal = true
-    }


cc @lhc70000 did you intend to do something with this

uiryuu · 2023-07-15T02:03:21Z

iina/Extensions.swift

-
-  func add(_ multiplier: CGFloat) -> NSSize {
-    return NSSize(width: width + multiplier, height: height + multiplier)
+


Remove spaces

uiryuu · 2023-07-15T02:08:59Z

iina/InfoDictionary.swift

-  /// A Boolean value that indicates whether this executable was an optimized (not debug) build.
-  #if DEBUG
-  let isDebug = true
-  #else
-  let isDebug = false
-  #endif
-


Let keep this, although there isn't any code using this. We might want to use this in the future.

uiryuu · 2023-07-15T02:10:13Z

iina/MPVController.swift

@@ -1394,7 +1360,7 @@ class MPVController: NSObject {
  }
 }

-fileprivate func mpvGetOpenGLFunc(_ ctx: UnsafeMutableRawPointer?, _ name: UnsafePointer<Int8>?) -> UnsafeMutableRawPointer? {
+fileprivate func mpvGetOpenGLFunc(_: UnsafeMutableRawPointer?, _ name: UnsafePointer<Int8>?) -> UnsafeMutableRawPointer? {


+1 for not changing

uiryuu · 2023-07-15T02:11:47Z

iina/ViewLayer.swift

@@ -195,7 +195,7 @@ class ViewLayer: CAOpenGLLayer {
  }

  // MARK: Utils
-
+  


uiryuu

Just drop some comments here, will take a deeper look another day. Merge conflicts look scary but are surprisingly trivial.

uiryuu · 2024-05-28T10:54:02Z

iina/Aspect.swift

@@ -11,7 +11,7 @@ import Cocoa
 class Aspect: NSObject {

  private var size: NSSize!
-


uiryuu · 2024-05-28T10:54:16Z

iina/Aspect.swift

@@ -20,7 +20,7 @@ class Aspect: NSObject {
      size.width = newValue
    }
  }
-


uiryuu · 2024-05-28T10:57:24Z

iina/VideoView.swift

+  _: CVDisplayLink, _: UnsafePointer<CVTimeStamp>,
+  _: UnsafePointer<CVTimeStamp>,
+  _: CVOptionFlags,
+  _: UnsafeMutablePointer<CVOptionFlags>,
  _ context: UnsafeMutableRawPointer?) -> CVReturn {


I'd like to keep it as it is.

chore: Stripped dead code

8203225

low-batt requested changes May 21, 2023

View reviewed changes

low-batt requested a review from uiryuu May 21, 2023 19:53

Semty added 3 commits May 21, 2023 22:02

fix: Returned auto-generated mpv wrappers to their base

ce4a8bc

fix: Returned a Check OpenGL error debug code in ViewLayer

c6e3bbc

fix: Returned a logger sub shooter code to Shooter

6b202ba

saagarjha requested changes May 23, 2023

View reviewed changes

Semty added 2 commits May 23, 2023 21:33

fix: Returned some convenience variables for Aspect

1d610bd

fix: Returned add(_ value:) extension function for NSSize

676b284

uiryuu requested changes Jul 15, 2023

View reviewed changes

uiryuu requested changes May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Stripped dead code #4425

chore: Stripped dead code #4425

Semty commented May 19, 2023

low-batt commented May 19, 2023

Semty commented May 20, 2023

low-batt left a comment

low-batt May 21, 2023

uiryuu Jul 15, 2023

low-batt May 21, 2023

Semty May 21, 2023

low-batt May 21, 2023

uiryuu May 28, 2024

saagarjha left a comment

saagarjha May 23, 2023

saagarjha May 23, 2023

Semty May 23, 2023 •

edited

Loading

saagarjha Jun 3, 2023

saagarjha May 23, 2023

low-batt May 26, 2023

uiryuu Jul 15, 2023

saagarjha May 23, 2023

uiryuu Jul 15, 2023

uiryuu Jul 15, 2023

uiryuu Jul 15, 2023

uiryuu Jul 15, 2023

uiryuu left a comment

uiryuu May 28, 2024

uiryuu May 28, 2024

uiryuu May 28, 2024

		/** Tags for "Open File/URL" menu item when "Always open file in new windows" is off. Vice versa. */
		fileprivate let NormalMenuItemTag = 0


		func add(_ multiplier: CGFloat) -> NSSize {
		return NSSize(width: width + multiplier, height: height + multiplier)

		@@ -195,7 +195,7 @@ class ViewLayer: CAOpenGLLayer {
		}

		// MARK: Utils

		@@ -11,7 +11,7 @@ import Cocoa
		class Aspect: NSObject {

		private var size: NSSize!

chore: Stripped dead code #4425

Are you sure you want to change the base?

chore: Stripped dead code #4425

Conversation

Semty commented May 19, 2023

low-batt commented May 19, 2023

Semty commented May 20, 2023

low-batt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saagarjha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Semty May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uiryuu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Semty May 23, 2023 •

edited

Loading