-
Notifications
You must be signed in to change notification settings - Fork 47
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
Ability to hide debug menu items #16
base: main
Are you sure you want to change the base?
Conversation
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.
Thank you for making these changes, they look great!! I have a some feedback about the API design here.
|
||
if !hasShortened && !isCharacterLimitWarningDisabled { | ||
hasShortened = true | ||
Swift.print("LocalConsole's content has exceeded 50,000 characters.\nTo maintain performance, LCManager cuts down the beginning of the printed content. To disable this behaviour, set LCManager.shared.isCharacterLimitDisabled to true.\nTo disable this warning, set LCManager.shared.isCharacterLimitWarningDisabled = true.") |
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.
I like this idea, but I think we can implement it in a slightly different way. Let's add a string to the top of the console log itself if it has been cut off, so that if you scroll to the very top of a shortened log, you see that message explaining how to disable the character limit.
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.
Hey @duraidabdul, sorry I didn't add this code, this is existing code I moved up to be within the Public marker rather than Private since it's public (or vice versa moved private code out of public marker).
Sources/LocalConsole/LCManager.swift
Outdated
public var showDebugMenuItem = true { | ||
didSet { menuButton.menu = makeMenu() } | ||
} | ||
public var showUserDefaultsMenuItem = true { |
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.
Instead of multiple properties, let's just do one property called showDebugMenu
and default to true
. We don't need the additional complexity of having multiple toggles for each item, all the debug menu items are hidden in a submenu anyway.
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.
@duraidabdul That would work for me, in our app we'd be okay hiding the Debug menu and thus all its menu items altogether. But wanted to check, you don't feel there's a use case to have the Debug menu but remove some of its items? For example, an app may want the option to log a system report but don't want them to be able to see the user defaults, don't want an option to restart springboard, etc?
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.
This extra customization isn't really needed because all of the debug menu items are intended to be used for debugging only, they should never be exposed to a user (this is because they use swizzling & private API, and exploit a bug to crash backboardd).
Some new public vars to customize which debug menu items are included :) #15