Created attachment 316837 [details] [review]
chatView: change color of status header on hover

Review of attachment 316837 [details] [review]:

::: src/chatView.js
@@ +152,3 @@
         this._pendingLogs = [];
         this._statusCount = { left: 0, joined: 0, total: 0 };
+        this._hoveredtags = []; // 'status-compressed' tags currently hovered

I'm not sure I understand the need of an array here. Shouldn't there only ever need to be one tag needing style (the status-compressed* one, where * is a number)?

@@ +262,3 @@
 
+        this._blackColor = new Gdk.RGBA ();
+        this._blackColor.parse('black');

This only works if you are using default theme or a light theme, but you probably dont want to use a black color as such if you have a dark theme like Adwaita dark (you can test it with gnome-tweak-tool, turning on a global dark theme).

I suggest you do something similar to how the dimColor is loaded in line 240-245. Instead of using the "dim-label" class you can use the "view" class to get hold of a pure black (or pure white in case of dark theme).

(In reply to Bastian Ilsø from comment #2)
> Review of attachment 316837 [details] [review] [review]:
> 
> ::: src/chatView.js
> @@ +152,3 @@
>          this._pendingLogs = [];
>          this._statusCount = { left: 0, joined: 0, total: 0 };
> +        this._hoveredtags = []; // 'status-compressed' tags currently
> hovered
> 
> I'm not sure I understand the need of an array here. Shouldn't there only
> ever need to be one tag needing style (the status-compressed* one, where *
> is a number)?
> 

I was thinking of multi touch devices. But since I am clearing out style at each function call, array won't be needed. 

> @@ +262,3 @@
>  
> +        this._blackColor = new Gdk.RGBA ();
> +        this._blackColor.parse('black');
> 
> This only works if you are using default theme or a light theme, but you
> probably dont want to use a black color as such if you have a dark theme
> like Adwaita dark (you can test it with gnome-tweak-tool, turning on a
> global dark theme).
> 
> I suggest you do something similar to how the dimColor is loaded in line
> 240-245. Instead of using the "dim-label" class you can use the "view" class
> to get hold of a pure black (or pure white in case of dark theme).

I had this in mind. I am having trouble finding documentation or reference on inbuilt CSS classes and styles provided by GNOME.

Created attachment 316864 [details] [review]
chatView: change color of status header on hover

On dark theme, strangely for me dim-label class and view class has almost same font color.

(In reply to Kunaal Jain from comment #4)
> On dark theme, strangely for me dim-label class and view class has almost
> same font color.

ah yes, well the issue seems to be in the theme, as the dim-labels are using opacity to make them grey rather than a blend..

also, found one other more issue. If you happen to hover over a status message while changing the view, then the hover is never reset to use the dim color. I wonder if there is a way we can handle that nicely.

Beyond that, I don't have more comments on the code, (Florian might need to chime in here before we push anything, though).

On commit message:


> On idle channels, status messages are compressed into
> 'status header' message. By clicking status header, all
> status messages are revealed.

I think we can leave this part out of the commit message.


> Changes the on hover color
> of status messages from dimcolor to black which helps in
> indicating the link is clickable.

I don't we need to mention the color as that now depends on the theme here. I suggest change to:
"Highlight the compressed status messages on hover by changing the color. The change helps indicating that the status message can be clicked."

Created attachment 316969 [details] [review]
chatView: change color of status header on hover

fixes hovering not reverting back when changing the view. Commit message updated too ;)

Review of attachment 316969 [details] [review]:

As mentioned on IRC/mail, all the different treatment of links/status headers in _handleLinkClicks/Hovers is getting messy, so I've refactored stuff a bit in bug 759677 - it probably makes sense to rebase this patch on top of that.

::: src/chatView.js
@@ +252,3 @@
 
+        context.save();
+        context.add_class('view');

Mmmh, maybe we should just use the TextView's style context in the first place? None of the stuff we're fetching from the context actually make sense for a ScrolledWindow ...

@@ +304,3 @@
                                     right_margin: MARGIN });
+        this._view.add_events(Gdk.EventMask.LEAVE_NOTIFY_MASK);
+        this._view.add_events(Gdk.EventMask.ENTER_NOTIFY_MASK);

If you rebase on top of bug 759677, it will make sense to split this out

Created attachment 317795 [details] [review]
chatView: change color of status header on hover

Created attachment 317796 [details] [review]
chatView: Listen to crossover events

Created attachment 317797 [details] [review]
chatView: change color of status header on hover

sorry for the noise folks.

Created attachment 317800 [details] [review]
chatView: change color of status header on hover

as pointed out by florian, on style update we are not updating the status compressed tags.

Review of attachment 317796 [details] [review]:

Actually using the enter/leave events to update the hover status belongs here as well, not in the other patch.

(Commit message could be better as well and mention *why* we should do this, but I won't insist on it if you just want to update the patch)

Review of attachment 317800 [details] [review]:

::: src/chatView.js
@@ +346,3 @@
+        context.add_class('view');
+        context.set_state(Gtk.StateFlags.NORMAL);
+        this._highlightColor = context.get_color(context.get_state());

highlightColor is a bit generic - statusHeaderHoverColor maybe?

@@ +412,3 @@
+                           Lang.bind(this, this._handleButtonTagsHover));
+        this._view.connect('leave-notify-event',
+                           Lang.bind(this, this._handleButtonTagsHover));

This belongs in the previous patch

@@ +876,3 @@
 
+            headerTag.connect('notify::hover', Lang.bind(this,
+            function() {

Style nit: missing indentation

@@ +877,3 @@
+            headerTag.connect('notify::hover', Lang.bind(this,
+            function() {
+                headerTag.foreground_rgba = headerTag.hover? this._highlightColor : null;

Style nit: missing space before '?'

Created attachment 317806 [details] [review]
chatView: Listen to crossover events

Created attachment 317807 [details] [review]
chatView: change color of status header on hover

Review of attachment 317806 [details] [review]:

LGTM

Review of attachment 317807 [details] [review]:

Dito

Review of attachment 317806 [details] [review]:

Pushed to master with commit 8b4f719f