- 
                Notifications
    
You must be signed in to change notification settings  - Fork 59
 
Remove the other Color type. The idea is to use AzColor for everything. #141
Conversation
        
          
                src/azure.rs
              
                Outdated
          
        
      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 really don't think we should have a method called "hack."
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.
Yep, I agree. How you like to have it named?
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.
Why is it necessary? It's unclear why you need to create a new AzColor where it's used.
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.
Legacy code calls converting the Color to AzColor and vice-versa. I will
try to remove them.
On Mon, Feb 9, 2015 at 12:38 PM, Martin Robinson notifications@github.com
wrote:
In src/azure.rs
#141 (comment):@@ -261,6 +261,16 @@ impl PartialEq for AzColor {
}
}+impl AzColor {
- pub fn new(r: AzFloat, g: AzFloat, b: AzFloat, a: AzFloat) -> AzColor {
 AzColor { r: r, g: g, b: b, a: a }- }
 
- pub fn hack(color: AzColor) -> AzColor {
 Why is it necessary? It's unclear why you need to create a new AzColor
where it's used.—
Reply to this email directly or view it on GitHub
https://github.com/servo/rust-azure/pull/141/files#r24363008.
| 
           Yeah, there is a need to make an explicit copy of a immutable AzColor to pass it to C code. I will rename the function to 'copy' as it better describes it use.  | 
    
        
          
                src/azure.rs
              
                Outdated
          
        
      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.
If you derive from Copy do you really need to implement it as well?
e7210b2    to
    6a7c324      
    Compare
  
    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.
Do you need to add Copy since you are simply using clone below?
| 
           This looks good to me.  | 
    
| 
           I'm not a huge fan of the   | 
    
6a7c324    to
    d7c59da      
    Compare
  
    | 
           Agreed. Please check the updated patch.  | 
    
(i.e. AzColor). This patch will also move the auxiliar new() function to inside Servo (together with rgb(), rgba(), black(), etc). Also addresses reviewer's comment about AzColor.
d7c59da    to
    df57406      
    Compare
  
    | 
           Needs rebase, will close this request and open a new one.  | 
    
No description provided.