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

Fix group icon border #3094

Merged
merged 6 commits into from May 26, 2022
Merged

Fix group icon border #3094

merged 6 commits into from May 26, 2022

Conversation

Coeur
Copy link
Collaborator

@Coeur Coeur commented May 15, 2022

Fix #3080

@sweetppro
Copy link
Collaborator

on line 392 under - (NSImage*)imageForGroupNone:
[NSColor.blackColor setStroke]; should perhaps be [NSColor.controlColor setStroke]; or [NSColor.controlTextColor setStroke]; to account for Dark Mode
:)

@Coeur
Copy link
Collaborator Author

Coeur commented May 15, 2022

Thanks @sweetppro. I'm a bit confused on the differences between controlTextColor, windowFrameTextColor and labelColor, as they look similar. I'll adopt controlTextColor as you suggested.

@GaryElshaw
Copy link
Contributor

That's enormous! You trying to induce seizures again, Antoine?
SCR-20220516-4a0

@Coeur
Copy link
Collaborator Author

Coeur commented May 15, 2022

@GaryElshaw
Kindly help me to understand if it was a compliment or a report for an issue?
Do you mean the size is too big? If yes, can you let me know your macOS version and display hardware?
If you return the ICON_WIDTH * 2 back to ICON_WIDTH, is that resolving it for you?

@GaryElshaw
Copy link
Contributor

GaryElshaw commented May 15, 2022

@GaryElshaw Kindly help me to understand if it was a compliment or a report for an issue? Do you mean the size is too big? If yes, can you let me know your macOS version and display hardware?

Yes, i was joking, but they are too big. All of the icons in Transmission are built at 32x32px or 16x16px. Often both sizes. I'm not sure what size that is, but it's certainly far bigger than that.

Also, note how the images have been cut off at the sides? I assume that's because of the initial parameters that will probably be 32x32.

I'm on a MacBook Air (Intel HD Graphics 5000 1536 MB) with Monterey 12.3.1

I was using the binary built on TeamCity.

@Coeur
Copy link
Collaborator Author

Coeur commented May 15, 2022

I made an experimental change with convertRectFromBacking:, while it might need the opposite (convertRectToBacking:), or get ride of the * 2 altogether. I don't have the hardware to verify myself, so @GaryElshaw kindly try it.

@GaryElshaw
Copy link
Contributor

GaryElshaw commented May 15, 2022

@Coeur Still the same, Antoine.

SCR-20220516-5sv

Here is a 32x32 icon
GreyDotFlat@2x

@GaryElshaw
Copy link
Contributor

GaryElshaw commented May 15, 2022

Sorry Antoine, that looks the same to me :-(

@Coeur Coeur force-pushed the coeur/groupColorBorder branch
SCR-20220516-6xr

Just my opinion, but i don't think there was anything wrong with your original images, albeit i thought they were a little large. I don't think they need a border, but size is important. #3065 (comment)

@Coeur
Copy link
Collaborator Author

Coeur commented May 15, 2022

@GaryElshaw thank you for trying. I'm giving up on the * 2 scale support for now. Please tell me it's the correct size on the latest commit.

@GaryElshaw
Copy link
Contributor

GaryElshaw commented May 16, 2022

Please tell me it's the correct size on the latest commit.

@Coeur It is, and better sizing than before! Except that ugly dashed circle icon is back. :-(

SCR-20220516-kxf

@GaryElshaw
Copy link
Contributor

@Coeur @sweetppro Ladies and Gentlemen, i present the ultimate fix for #3067 I fixed it!

circle-cut

@sweetppro
Copy link
Collaborator

sweetppro commented May 16, 2022

this looks good to me on Monterey:
*perhaps in light mode the none icon could be slightly thicker

- (NSImage*)imageForGroupNone
{
    static NSImage* icon;
    if (icon)
    {
        return icon;
    }

    icon = [NSImage imageWithSize:NSMakeSize(ICON_WIDTH, ICON_WIDTH) flipped:NO drawingHandler:^BOOL(NSRect rect) {
        //border
        rect = NSInsetRect(rect, 0.5, 0.5);
        NSBezierPath* bp = [NSBezierPath bezierPathWithRoundedRect:rect xRadius:rect.size.width / 2 yRadius:rect.size.width / 2];
        bp.lineWidth = 1;
        [NSColor.secondaryLabelColor setStroke];
        [bp stroke];

        return YES;
    }];

    return icon;
}

- (NSImage*)imageForGroup:(NSMutableDictionary*)dict
{
    NSImage* icon;
    if ((icon = dict[@"Icon"]))
    {
        return icon;
    }

    NSColor* color = dict[@"Color"];
    NSColor* borderColor = [color blendedColorWithFraction:0.3 ofColor:NSColor.controlTextColor];
    if ([self appearanceIsDark])
    {
        borderColor = [color blendedColorWithFraction:0.15 ofColor:NSColor.controlTextColor];
    }

    icon = [NSImage imageWithSize:NSMakeSize(ICON_WIDTH, ICON_WIDTH) flipped:NO drawingHandler:^BOOL(NSRect rect) {
        //border
        NSBezierPath* bp = [NSBezierPath bezierPathWithRoundedRect:rect xRadius:rect.size.width / 2 yRadius:rect.size.width / 2];
        bp.lineWidth = 0;
        NSGradient* gradient = [[NSGradient alloc] initWithStartingColor:borderColor
                                                             endingColor:borderColor];
        [gradient drawInBezierPath:bp angle:0.0];

        //inside
        rect = NSInsetRect(rect, 1.0, 1.0);
        bp = [NSBezierPath bezierPathWithRoundedRect:rect xRadius:rect.size.width / 2 yRadius:rect.size.width / 2];
        bp.lineWidth = 0;
        [color setFill];
        [bp fill];

        return YES;
    }];

    dict[@"Icon"] = icon;

    return icon;
}

- (BOOL) appearanceIsDark
{
    if (@available(macOS 10.14, *))
    {
        NSAppearance *appearance = [NSApp effectiveAppearance];
        NSAppearanceName basicAppearance = [appearance bestMatchFromAppearancesWithNames:@[NSAppearanceNameAqua,NSAppearanceNameDarkAqua]];
    
        if ([basicAppearance isEqualToString:NSAppearanceNameDarkAqua])
        {
            return YES;
        }
    }
    return NO;
}

finder_light
tr_light
finder_dark
tr_dark

@GaryElshaw
Copy link
Contributor

GaryElshaw commented May 16, 2022

Yes please, this version @Coeur @sweetppro

image

I've closed my PR and will redo it with just the Message Log icons.

@GaryElshaw
Copy link
Contributor

Can you include the changes @sweetppro made that include the new empty icon from my previous post, Antoine? That empty icon finally rids the dashed element still in the latest build.
SCR-20220516-tro

@sweetppro
Copy link
Collaborator

There is no gradient on Monterey either

@Coeur
Copy link
Collaborator Author

Coeur commented May 16, 2022

Yes, I'll look at sweetppro colors (maybe in a few hours).
But I'll keep the dashed style for now, to avoid confusion with a custom group color that would be light grey.

@sweetppro
Copy link
Collaborator

@Coeur The Monterey Finder has a solid circle for no tag. the code snippet I posted above does get pretty close to Monterey's Finder tags:
168518561-9bf1db04-ca3f-44d7-a059-0cf001a36137

@Coeur
Copy link
Collaborator Author

Coeur commented May 16, 2022

@sweetppro the last one is not the "no group", it's a visual glitch. Tap on it, and it will update its color to grey:
Capture d’écran 2022-05-16 à 20 07 59

@sweetppro
Copy link
Collaborator

sweetppro commented May 16, 2022

@sweetppro the last one is not the "no group", it's a visual glitch. Tap on it, and it will update its color to grey:
Capture d’écran 2022-05-16 à 20 07 59

oh haha,
so it is, you're right.

I think it maybe does look nicer with a solid outline though

@GaryElshaw
Copy link
Contributor

@Coeur The Monterey Finder has a solid circle for no tag. the code snippet I posted above does get pretty close to Monterey's Finder tags: 168518561-9bf1db04-ca3f-44d7-a059-0cf001a36137

Yep, which was my intention all along @sweetppro

@sweetppro
Copy link
Collaborator

@Coeur e9ec03c is looking great
:)

@Coeur
Copy link
Collaborator Author

Coeur commented May 16, 2022

There is no gradient on Monterey either

After zooming 10 times, OK, I can confirm this. I've applied your border colour suggestion.

@GaryElshaw
Copy link
Contributor

There is no gradient on Monterey either

After zooming 10 times, OK, I can confirm this. I've applied your border colour suggestion.

Looks great! (except that poxy-dashed-thing :-)

@sweetppro
Copy link
Collaborator

that same dot to left of the file icon in the main window should look great :)
(just like Finder tags)

@sweetppro
Copy link
Collaborator

Looks great! (except that poxy-dashed-thing :-)

its not so bad :)

@GaryElshaw
Copy link
Contributor

Looks great! (except that poxy-dashed-thing :-)

its not so bad :)

Yes it is! :-)

@sweetppro
Copy link
Collaborator

Looks great! (except that poxy-dashed-thing :-)

its not so bad :)

Yes it is! :-)

ha!
v4 is shaping up to be a pretty sweet upgrade,
looking forward to an official release

@ckerr
Copy link
Member

ckerr commented May 16, 2022

@Coeur from the discussion I think the consensus is this PR is ready to be merged but I'm not positive and don't want to merge it prematurely. Please give me an @ in here when it's ready and I'm happy to merge it

@GaryElshaw
Copy link
Contributor

GaryElshaw commented May 16, 2022

I'm happy to merge it

Gah!

Please reconsider that Empty icon @Coeur, it is the only one of its kind in Transmission (there are no other dashed icons), and it goes against the grain of trying to emulate the tags in MacOS that are theoretically familiar to all MacOS users. I'd understand it if the icon didn't have a label, but it is clearly labelled as 'Empty'.

@Coeur
Copy link
Collaborator Author

Coeur commented May 16, 2022

@ckerr in regard of @GaryElshaw request, I've adopted a linear style instead of a dashed style. The code is good to merge once we get @GaryElshaw or @CyberSkull confirmation.

@sweetppro
Copy link
Collaborator

@Coeur I think the border width is a touch too thin for the groupNone, also the color is too dark/bright on the group filter bar.
NSColor.tertiaryLabelColor looks good on the filter bar (but not in the menu....)

and the following looks good for the line width (I think)

rect = NSInsetRect(rect, BORDER_WIDTH * 3 / 4, BORDER_WIDTH * 3 / 4);
        NSBezierPath* bp = [NSBezierPath bezierPathWithOvalInRect:rect];
        bp.lineWidth = BORDER_WIDTH * 4 / 3;

light
dark

@Coeur
Copy link
Collaborator Author

Coeur commented May 16, 2022

OK, then give me an extra day to look at those adjustment suggestions, @sweetppro.

Replace GroupsNoneTemplace.png with a dynamically generated icon.
@sweetppro
Copy link
Collaborator

@Coeur it looks like the image in AddMagnetWindow.xib also needs to be updated to reflect the non dashed image change

@Coeur
Copy link
Collaborator Author

Coeur commented May 17, 2022

It's a placeholder in this xib file. It doesn't matter what you put in.

@Coeur
Copy link
Collaborator Author

Coeur commented May 17, 2022

@Coeur I think the border width is a touch too thin for the groupNone, also the color is too dark/bright on the group filter bar. NSColor.tertiaryLabelColor looks good on the filter bar (but not in the menu....)

You suggested a border width of 1.33 for None, and 1 for the rest. I did 1.25 for all in my latest commit, as I would prefer to have consistent thickness. Let me know if it looks fine.
controlTextColor seems the best compromise according to Color Meter, so I don't think it's necessary to change it to secondaryLabelColor or tertiaryLabelColor.

@sweetppro
Copy link
Collaborator

@Coeur I think the border width is a touch too thin for the groupNone, also the color is too dark/bright on the group filter bar. NSColor.tertiaryLabelColor looks good on the filter bar (but not in the menu....)

You suggested a border width of 1.33 for None, and 1 for the rest. I did 1.25 for all in my latest commit, as I would prefer to have consistent thickness. Let me know if it looks fine. controlTextColor seems the best compromise according to Color Meter, so I don't think it's necessary to change it to secondaryLabelColor or tertiaryLabelColor.

I’m away from my computer till lunchtime. I’ll take a look then

@sweetppro
Copy link
Collaborator

@Coeur This looks good, but I also propose the following to fix the groupNon Color on the groups filter bar:
GUI > Sources > Controller.mm:3450
change:

else if ([ident isEqualToString:@"Color"])
        {
            NSInteger groupIndex = group.groupIndex;
            return [GroupsController.groups imageForIndex:groupIndex];
        }

to:

else if ([ident isEqualToString:@"Color"])
        {
            NSInteger groupIndex = group.groupIndex;
            NSImage *icon = [GroupsController.groups imageForIndex:groupIndex];
            if (groupIndex == -1)
            {
                [icon setTemplate:YES];
            }
            return icon;
        }

group_filter

@GaryElshaw
Copy link
Contributor

Out of curiosity, why 11.0 and not 12.0?

SCR-20220519-p06

@Coeur
Copy link
Collaborator Author

Coeur commented May 19, 2022

[edit: @GaryElshaw let's continue on #3112]

@GaryElshaw
Copy link
Contributor

@Coeur Ironically.... #3112

@sweetppro
Copy link
Collaborator

@Coeur i can submit a new PR for this fix if you’d prefer that.

group_filter

but this PR as far as I can see looks great
😊

@Coeur
Copy link
Collaborator Author

Coeur commented May 19, 2022

@sweetppro change done.
@ckerr good to merge. :)

@GaryElshaw

This comment was marked as duplicate.

@GaryElshaw

This comment was marked as duplicate.

@ckerr ckerr merged commit b033b65 into transmission:main May 26, 2022
ckerr pushed a commit that referenced this pull request May 26, 2022
@GaryElshaw
Copy link
Contributor

Thanks Charles! @ckerr

@Coeur Coeur deleted the coeur/groupColorBorder branch June 4, 2022 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add border to group colors
4 participants