Closed Bug 954541 Opened 10 years ago Closed 10 years ago

Contact list section header styling for Linux

Categories

(Instantbird Graveyard :: Contacts window, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

(Whiteboard: [1.2-wanted])

Attachments

(3 files, 6 obsolete files)

*** Original post on bio 1107 at 2011-10-21 17:44:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached file Patch for instantbird/themes/blist.css (obsolete) —
*** Original post on bio 1107 as attmnt 918 at 2011-10-21 17:44:00 UTC ***

This is a Linux adaptation of Mic's Windows/Aero patch to bug 954413 (bio 979). I experimented with the gradient Mic added to the section headers, but it was hard to find something which fits in across various GTK/KDE themes. I ended up settling for some subtle borders, which looks quite good I think.
Attachment #8352660 - Flags: review?
Attached image Screenshot
*** Original post on bio 1107 as attmnt 919 at 2011-10-21 17:45:00 UTC ***

Some screenshots. 

Someone should probably check how this looks on the latest ubuntu before accepting the patch!
Comment on attachment 8352660 [details]
Patch for instantbird/themes/blist.css

*** Original change on bio 1107 attmnt 918 at 2011-10-25 16:04:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352660 - Attachment is patch: false
Attachment #8352660 - Attachment mime type: text/plain → text/css
*** Original post on bio 1107 as attmnt 982 at 2011-11-09 01:02:00 UTC ***

Now a diff - not sure what went wrong there.
Attachment #8352723 - Flags: review?(florian)
Comment on attachment 8352660 [details]
Patch for instantbird/themes/blist.css

*** Original change on bio 1107 attmnt 918 at 2011-11-09 01:02:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352660 - Attachment is obsolete: true
Attachment #8352660 - Flags: review?
Whiteboard: [1.2-wanted]
Comment on attachment 8352723 [details] [diff] [review]
Patch for instantbird/themes/blist.css

*** Original change on bio 1107 attmnt 982 at 2012-01-30 11:24:47 UTC ***

I'm not sure of why a patch for a Linux-only bug changes the CSS for non-Mac (that is both Linux and Windows), but that doesn't look like what you intended to do.
Attachment #8352723 - Flags: review?(florian) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1107 as attmnt 1142 at 2012-01-30 12:36:00 UTC ***

Back in October when I did this, I thought XP_MACOSX referred to Windows (XP, i.e. non-Aero) and OSX ;) Shame there is not XP_LINUX, it would make things more legible.
Attachment #8352887 - Flags: review?(florian)
Comment on attachment 8352723 [details] [diff] [review]
Patch for instantbird/themes/blist.css

*** Original change on bio 1107 attmnt 982 at 2012-01-30 12:36:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352723 - Attachment is obsolete: true
*** Original post on bio 1107 at 2012-01-30 13:18:41 UTC ***

Comment on attachment 8352887 [details] [diff] [review] (bio-attmnt 1142)
Patch

>+%ifndef XP_MACOSX
>+%ifndef XP_WIN

I suggest you use this instead as it only involves OSs that have XP_UNIX defined:

%ifdef XP_UNIX
%ifndef XP_MACOSX
Comment on attachment 8352887 [details] [diff] [review]
Patch

*** Original change on bio 1107 attmnt 1142 at 2012-04-24 16:52:29 UTC ***

Per Mic's suggestion.

For the actual look of it, I think it looks fine (and I think you're nominally in charge of our *NIX theme, so...I'm OK with the changes).
Attachment #8352887 - Flags: review?(florian) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1107 as attmnt 1390 at 2012-04-24 16:57:00 UTC ***

OK
Attachment #8353143 - Flags: review?(clokep)
Comment on attachment 8352887 [details] [diff] [review]
Patch

*** Original change on bio 1107 attmnt 1142 at 2012-04-24 16:57:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352887 - Attachment is obsolete: true
Comment on attachment 8353143 [details] [diff] [review]
Patch

*** Original change on bio 1107 attmnt 1390 at 2012-04-24 17:03:10 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353143 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1107 as attmnt 1391 at 2012-04-24 17:06:00 UTC ***

Typo fix.
Attachment #8353144 - Flags: review?(clokep)
Comment on attachment 8353143 [details] [diff] [review]
Patch

*** Original change on bio 1107 attmnt 1390 at 2012-04-24 17:06:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353143 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1107 as attmnt 1402 at 2012-04-25 11:38:00 UTC ***

Changes now integrated into existing ifdef tree. Not sure why I didn't do that originally.
Attachment #8353155 - Flags: review?(clokep)
Comment on attachment 8353144 [details] [diff] [review]
Patch

*** Original change on bio 1107 attmnt 1391 at 2012-04-25 11:38:08 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353144 - Attachment is obsolete: true
Attachment #8353144 - Flags: review?(clokep)
*** Original post on bio 1107 at 2012-04-25 11:49:18 UTC ***

Comment on attachment 8353155 [details] [diff] [review] (bio-attmnt 1402)
Patch

>--- blist-old.css	2012-04-25 12:08:09.000000000 +0200
>+++ blist.css	2012-04-25 13:33:02.000000000 +0200
>@@ -68,39 +68,53 @@
>   border-bottom-color: rgba(0, 0, 0, 0.32);
> %else
>   background-color: -moz-Dialog;
> %ifdef XP_WIN
>   background-image: -moz-linear-gradient(rgba(255,255,255,.5), rgba(255,255,255,0));
> %else
>   background-image: -moz-linear-gradient(rgba(255,255,255,.3), rgba(255,255,255,0));
> %endif
>+%ifndef XP_UNIX
> %ifndef XP_WIN
If not XP_UNIX, if not XP_WIN...what's left?! (But seriously, I think you might be able to just remove this ifdef then?)

>   border-bottom: 1px solid ThreeDShadow;
> %else
>   border-bottom: none;
> %endif
> %endif
>+%endif
> }


> #contactsHeader {
> %ifdef XP_MACOSX
>   border-top: 1px solid rgb(66, 66, 66);
> }
> 
> #contactsHeader:-moz-window-inactive {
>   border-top-color: rgb(137, 137, 137);
> %else
>+%ifdef XP_UNIX
>+  border-top: 1px solid rgba(0, 0, 0, 0.1);
>+%else
> %ifndef XP_WIN
>   border-top: 1px solid ThreeDShadow;
> %else
>   border-top: none;
> %endif
This seems confusing for me, I know it's not XP_MACOSX, then we have a statement (you added) for XP_UNIX, then we have not XP_WIN and other. So it seems like there's two things being set for UNIX.

> %endif
>+%endif
>+}
>+
>+%ifdef XP_UNIX
>+%ifndef XP_MACOSX
>+#convsHeader {
>+  border-top: 1px solid rgba(0, 0, 0, 0.12);
> }
>+%endif
>+%endif


A flowchart would be useful. ;) :)
Attached patch PatchSplinter Review
*** Original post on bio 1107 as attmnt 1403 at 2012-04-25 13:16:00 UTC ***

> If not XP_UNIX, if not XP_WIN...what's left?! (But seriously, I think you might
> be able to just remove this ifdef then?)

I wasn't completely sure there weren't any obscure platforms I didn't know about. 

So this patch is a little simpler ;)
Attachment #8353156 - Flags: review?(clokep)
Comment on attachment 8353155 [details] [diff] [review]
Patch

*** Original change on bio 1107 attmnt 1402 at 2012-04-25 13:16:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353155 - Attachment is obsolete: true
Attachment #8353155 - Flags: review?(clokep)
*** Original post on bio 1107 at 2012-04-25 13:55:23 UTC ***

Comment on attachment 8353156 [details] [diff] [review] (bio-attmnt 1403)
Patch

(In reply to comment #11)
> Created attachment 8353156 [details] [diff] [review] (bio-attmnt 1403) [details]
> Patch
> > If not XP_UNIX, if not XP_WIN...what's left?! (But seriously, I think you might
> > be able to just remove this ifdef then?)
> I wasn't completely sure there weren't any obscure platforms I didn't know
> about. 
I don't think there's any we care about here, if there is...well we don't distribute them. :)

> -%ifndef XP_WIN
> -  border-bottom: 1px solid ThreeDShadow;
> -%else
> -  border-bottom: none;
> -%endif

Won't this change XP_WIN also?
*** Original post on bio 1107 at 2012-04-25 14:03:30 UTC ***

(In reply to comment #12)

> > -%ifndef XP_WIN
> > -  border-bottom: 1px solid ThreeDShadow;
> > -%else
> > -  border-bottom: none;
> > -%endif
> 
> Won't this change XP_WIN also?

Yes, but the only other XP_WIN border CSS I can see here is border-style: none, so it seems superfluous.
Comment on attachment 8353156 [details] [diff] [review]
Patch

*** Original change on bio 1107 attmnt 1403 at 2012-04-26 00:50:19 UTC ***

This patch looks a lot better. :) I can't really give a UI review since I don't have Linux. :( The screenshots look fine though.
Attachment #8353156 - Flags: review?(clokep) → review+
Attachment #8353156 - Flags: review?(florian)
Comment on attachment 8353156 [details] [diff] [review]
Patch

*** Original change on bio 1107 attmnt 1403 at 2012-07-17 15:11:33 UTC ***

I now have Linux, so setting this back to me for a UI review.
Attachment #8353156 - Flags: review?(florian)
Attachment #8353156 - Flags: review?(clokep)
Attachment #8353156 - Flags: review+
*** Original post on bio 1107 as attmnt 1771 at 2012-08-02 00:42:00 UTC ***

Here's how this looks under unity with and without the patch applied.
Comment on attachment 8353532 [details]
Screenshot under unity (left: with patch, right: without patch)

*** Original change on bio 1107 attmnt 1771 at 2012-08-02 00:48:09 UTC ***

Oops, I meant the left one is WITH the patch and the right one is WITHOUT the patch.
Attachment #8353532 - Attachment description: Screenshot under unity (left: without patch, right: with patch) → Screenshot under unity (left: with patch, right: without patch)
Comment on attachment 8353156 [details] [diff] [review]
Patch

*** Original change on bio 1107 attmnt 1403 at 2012-08-02 00:52:19 UTC ***

I think this is an improvement on all Linux desktop managers we tested.
Attachment #8353156 - Flags: review?(clokep) → review+
Whiteboard: [1.2-wanted] → [1.2-wanted][checkin-needed]
*** Original post on bio 1107 at 2012-08-02 18:10:20 UTC ***

Finally checked in as http://hg.instantbird.org/instantbird/rev/cdc65c66b724 Thanks! :) Sorry for the long review.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [1.2-wanted][checkin-needed] → [1.2-wanted]
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.