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.

Sign up for GitHub

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

Jump to bottom

Moved the default value formatter #3088

Merged
merged 5 commits into from
Jan 26, 2018
Merged

Moved the default value formatter #3088

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b64eda
Moved the default value formatter
jjatie Dec 11, 2017
9cf48c1
Merged latest 4.0.0
jjatie Jan 16, 2018
45207ae
Fixes after merge.
jjatie Jan 22, 2018
60b5b60
Merge remote-tracking branch 'upstream/4.0.0' into default-value-form…
jjatie Jan 26, 2018
e1628de
Merged latest 4.0.0
jjatie Jan 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Source/Charts/Data/Implementations/ChartBaseDataSet.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ open class ChartBaseDataSet: NSObject, ChartDataSetProtocol
{
if needsFormatter
{
return ChartUtils.defaultValueFormatter()
return DefaultValueFormatter()
}

return _valueFormatter
Expand Down
63 changes: 30 additions & 33 deletions Source/Charts/Formatters/DefaultValueFormatter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import Foundation

/// The default value formatter used for all chart components that needs a default
@objc(ChartDefaultValueFormatter)
open class DefaultValueFormatter: NSObject, ValueFormatter
{
Expand All @@ -22,69 +23,65 @@ open class DefaultValueFormatter: NSObject, ValueFormatter

@objc open var block: Block?

@objc open var hasAutoDecimals: Bool = false
@objc open var hasAutoDecimals: Bool
Copy link
Member

Choose a reason for hiding this comment

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

why remove default value here but initialize later in init() many times?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because it has different values based on which initializer. This way it's not assigned twice.

Copy link
Member

Choose a reason for hiding this comment

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

alright, makes sense then.


private var _formatter: NumberFormatter?
@objc open var formatter: NumberFormatter?
{
get { return _formatter }
set
{
@objc open var formatter: NumberFormatter? {
willSet {
hasAutoDecimals = false
_formatter = newValue
}
}

private var _decimals: Int?
open var decimals: Int?
{
get { return _decimals }
set
{
_decimals = newValue

if let digits = newValue
open var decimals: Int? {
didSet {
if let digits = decimals
{
self.formatter?.minimumFractionDigits = digits
self.formatter?.maximumFractionDigits = digits
self.formatter?.usesGroupingSeparator = true
formatter?.minimumFractionDigits = digits
formatter?.maximumFractionDigits = digits
formatter?.usesGroupingSeparator = true
}
}
}

public override init()
{
super.init()

self.formatter = NumberFormatter()
formatter = NumberFormatter()
formatter?.usesGroupingSeparator = true
Copy link
Member

Choose a reason for hiding this comment

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

why set usesGroupingSeparator and decimals here while the old code does not? did I miss anything

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made the empty initializer provide the same formatter that ChartUtils.defaultFormatter had which set it to true

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry. I missed the removal

decimals = 1
hasAutoDecimals = true

super.init()
}

@objc public init(formatter: NumberFormatter)
{
super.init()

self.formatter = formatter
hasAutoDecimals = false

super.init()
}

@objc public init(decimals: Int)
{
super.init()

self.formatter = NumberFormatter()
self.formatter?.usesGroupingSeparator = true
formatter = NumberFormatter()
formatter?.usesGroupingSeparator = true
self.decimals = decimals
hasAutoDecimals = true

super.init()
}

@objc public init(block: @escaping Block)
{
super.init()

self.block = block
hasAutoDecimals = false

super.init()
}

@objc public static func with(block: @escaping Block) -> DefaultValueFormatter?

/// This function is deprecated - Use `init(block:)` instead.
// DEC 11, 2017
@available(*, deprecated: 3.0, message: "Use `init(block:)` instead.")
@objc public static func with(block: @escaping Block) -> DefaultValueFormatter
{
return DefaultValueFormatter(block: block)
}
Expand Down
6 changes: 3 additions & 3 deletions Source/Charts/Utils/ChartUtils.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ extension Double
self != 0.0
else { return 0 }

let i = self.roundedToNextSignificant()
let i = roundedToNextSignificant()

guard
!i.isInfinite,
Expand Down Expand Up @@ -226,7 +226,7 @@ extension CGContext {
NSUIGraphicsPopContext()
}

internal func drawMultilineText(_ text: String, at point: CGPoint, constrainedTo size: CGSize, anchor: CGPoint, knownTextSize: CGSize, angleRadians: CGFloat, attributes: [NSAttributedStringKey : Any]?)
func drawMultilineText(_ text: String, at point: CGPoint, constrainedTo size: CGSize, anchor: CGPoint, knownTextSize: CGSize, angleRadians: CGFloat, attributes: [NSAttributedStringKey : Any]?)
Copy link
Member

Choose a reason for hiding this comment

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

how about we make these two drawMultilineText as open? I don't see why only internal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

First, it's always been internal. Second, we expose a way to extend the draw methods by wrapping them in another open method. I think it's safe to keep it internal. If we get requests to expose it, maybe we can change our mind then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, I don't think it makes sense to have any of these be open, because they would have to subclass CGContext which isn't recommended.

{
var rect = CGRect(origin: .zero, size: knownTextSize)

Expand Down Expand Up @@ -274,7 +274,7 @@ extension CGContext {
NSUIGraphicsPopContext()
}

internal func drawMultilineText(_ text: String, at point: CGPoint, constrainedTo size: CGSize, anchor: CGPoint, angleRadians: CGFloat, attributes: [NSAttributedStringKey : Any]?)
func drawMultilineText(_ text: String, at point: CGPoint, constrainedTo size: CGSize, anchor: CGPoint, angleRadians: CGFloat, attributes: [NSAttributedStringKey : Any]?)
{
let rect = text.boundingRect(with: size, options: .usesLineFragmentOrigin, attributes: attributes, context: nil)
drawMultilineText(text, at: point, constrainedTo: size, anchor: anchor, knownTextSize: rect.size, angleRadians: angleRadians, attributes: attributes)
Expand Down