I was going to file this bug but found your. This also happens to me when I listen to Rhythmbox and it skips to the next song while in overview. Window texts should resize when changed.

3.2.1. This bug should have the confirmed status.

(In reply to comment #1)
> This bug should have the confirmed status.

We don't really use that status - assume UNCONFIRMED == NEW == CONFIRMED

Anyhow.. Is this the original bug report or is there any one more detailed? Is this being worked on? I have seen this bug since 3.0 but I thought it would be fixed in 3.2.1. gtk_window_set_title or whatever it is called in gtk3 should do some kind of update to the shell overview. Is this a big thing or can it be fixed easily?

Created attachment 204371 [details] [review]
newbie patch

This patch fixes this bug to me, but there's an ugly workaround to get new width. Nonetheless, none of St.Label methods seems to me, that could solve this better. (sorry if this completely wrong, I'm new to fixing bugs and wanted to start somehow)

(In reply to comment #4)
> Created an attachment (id=204371) [details] [review]
> newbie patch
> 
> This patch fixes this bug to me, but there's an ugly workaround to get new
> width. Nonetheless, none of St.Label methods seems to me, that could solve this
> better. (sorry if this completely wrong, I'm new to fixing bugs and wanted to
> start somehow)

Nice work. It doesn't work fully though, the label isn't getting centred.

If you drag a changed title's window in the overview it resumes to the old size.

I did some modifications to keep the title centred:

//Get current position and width
let [x, y] = this.title.get_position();
let titleWidth = this.title.get_width();

//Determine and set new width
let helpTitle = new St.Label({ style_class: 'window-caption', text: w.title });
parentActor.add_actor(helpTitle);
this.title.set_width(helpTitle.get_width());

//Update position to keep title centred based on delta width
this.title.set_position(x + (titleWidth - this.title.get_width()) / 2, y);

If you grab a window and drop it again the title will revert but we are fixing :)

(In reply to comment #4)
> Created an attachment (id=204371) [details] [review]
> newbie patch
> 
> This patch fixes this bug to me, but there's an ugly workaround to get new
> width. Nonetheless, none of St.Label methods seems to me, that could solve this
> better. (sorry if this completely wrong, I'm new to fixing bugs and wanted to
> start somehow)

You don't have the latest gnome-shell 3.2.1, right? I kind of messed up my original workspace.js so I can't create a patch ^^

(In reply to comment #7)
> I did some modifications to keep the title centred:
> 
> //Get current position and width
> let [x, y] = this.title.get_position();
> let titleWidth = this.title.get_width();
> 
> //Determine and set new width
> let helpTitle = new St.Label({ style_class: 'window-caption', text: w.title });
> parentActor.add_actor(helpTitle);
> this.title.set_width(helpTitle.get_width());
> 
> //Update position to keep title centred based on delta width
> this.title.set_position(x + (titleWidth - this.title.get_width()) / 2, y);
> 
> If you grab a window and drop it again the title will revert but we are fixing
> :)

It should be a truncated position to avoid blurry text:

//Update position to keep title centred based on delta width
this.title.set_position(Math.floor(x + (titleWidth - this.title.get_width()) / 2), y);

Ah! We are not supposed to set the actual width but the fullWidth and let the shell decide if it should shrink it. If we only set the width, the fullWidth remains the same and is the width we get if the overview gets updated (window dragging, etc..). I only need an original workspace.js to create a working patch. Mine was in /usr/share/gnome-shell/js/ui. Could someone attach the original 3.2.1 please?

This works but is probably a crappy solution. The width of a new title should be capped to the thumbnail width - it isn't :P I can't find any documentation on how it's supposed to be done?


//Set new title text
this.title.text = w.title;

//Get current title position and width
let [titleX, titleY] = this.title.get_position();
let titleWidth = this.title.get_width();

//Determine and set new title width
let helpTitle = new St.Label({ style_class: 'window-caption', text: w.title });
parentActor.add_actor(helpTitle);
this.title.width = this.title.fullWidth = helpTitle.width;

//Center title based on delta width
this.title.set_position(Math.floor(titleX + (titleWidth - this.title.get_width()) / 2), titleY);

(In reply to comment #8)
> You don't have the latest gnome-shell 3.2.1, right? I kind of messed up my
> original workspace.js so I can't create a patch ^^

I'm not sure about what is "latest 3.2.1".. Well I have gnome-shell 3.2.1 and don't know if it's latest. Anyway, my patch was created aggainst latest version from gnome git repository.
http://git.gnome.org/browse/gnome-hell/plain/js/ui/workspace.js

That version is incompatible with my 3.2.1, but I've founded that out after I replaced my worskpace.js with latest version. I've fixed that up with 3.2 version of worskapce.js from gnome git repository, so it's pretty messy situation now and I don't have 3.2.1 version of worskapce.js

(In reply to comment #10)
> This works but is probably a crappy solution. The width of a new title should
> be capped to the thumbnail width - it isn't :P I can't find any documentation
> on how it's supposed to be done?

There's probably no documentation on this. Anyway, here's my solution:

this.title.fullWidth = helpTitle.width;
let windowCloneWidth = this._windowClone.actor.scale_x * this._windowClone.actor.get_width();
this.title.width = Math.min(this.title.fullWidth, windowCloneWidth);

Created attachment 204427 [details] [review]
patch adding updateTitlePosition method

Now I've found that bottom lines of updatePositions method of WindowOverlay do title repositioning job. I propose a new method _updateTitlePosition to avoid code duplicity.

This works fully but has some duplicated code. But isn't "notify::title" the only event that should update the title position anyway?

------

this.title.text = w.title;
let helpTitle = new St.Label({ style_class: 'window-caption', text: w.title });
parentActor.add_actor(helpTitle);
this.title.fullWidth = helpTitle.get_width();

let clone = this._windowClone;
let [cloneX, cloneY] = clone.actor.get_position();
let [cloneWidth, cloneHeight] = clone.actor.get_size();
cloneWidth *= clone.actor.scale_x;
cloneHeight *= clone.actor.scale_y;

this.title.width = Math.min(title.fullWidth, cloneWidth);
let titleX = cloneX + (cloneWidth - title.width) / 2;
let titleY = cloneY + cloneHeight + title._spacing;
title.set_position(Math.floor(titleX), Math.floor(titleY));

------

Is the helpTitle getting deleted when it runs out of scope? What does parentActor.add_actor(helpTitle); do really? Should we delete helpTitle explicitly?

(In reply to comment #12)
> Created an attachment (id=204427) [details] [review]
> patch adding updateTitlePosition method
> 
> Now I've found that bottom lines of updatePositions method of WindowOverlay do
> title repositioning job. I propose a new method _updateTitlePosition to avoid
> code duplicity.

Ah. Of course we should share the title positioning.

---

this.title.width = this.title.fullWidth = helpTitle.get_width();

should be

this.title.fullWidth = helpTitle.get_width();

since we set the actual width in the _updateTitlePosition

(In reply to comment #13)
> Is the helpTitle getting deleted when it runs out of scope? What does
> parentActor.add_actor(helpTitle); do really? 

I think, that we can't do helpTitle.get_width() if heplTitle isn't placed somewhere (shell crashed without this line).

> Should we delete helpTitle
> explicitly?

We probably should. I just haven't bothered with it, since it seems to be destroyed when leaving overview.

I read some Clutter docs and found clutter_container_remove_actor(). It removes the actor (helpTitle) and once the reference to it goes out of scope it gets deleted.

Here is my current code, you should make a patch sharing the code to update title position with updatePositions as you said.

this.title.text = w.title;
let helpTitle = new St.Label({ style_class: 'window-caption', text: w.title });
parentActor.add_actor(helpTitle);
this.title.fullWidth = helpTitle.get_width();
parentActor.remove_actor(helpTitle);

let clone = this._windowClone;
let [cloneX, cloneY] = clone.actor.get_position();
let [cloneWidth, cloneHeight] = clone.actor.get_size();
cloneWidth *= clone.actor.scale_x;
cloneHeight *= clone.actor.scale_y;

//This part should be shared with updatePositions
this.title.width = Math.min(title.fullWidth, cloneWidth);
let titleX = cloneX + (cloneWidth - title.width) / 2;
let titleY = cloneY + cloneHeight + title._spacing;
title.set_position(Math.floor(titleX), Math.floor(titleY));

Created attachment 204438 [details] [review]
patch

(In reply to comment #16)
> Here is my current code, you should make a patch sharing the code to update
> title position with updatePositions as you said.

(In reply to comment #17)
> Created an attachment (id=204438) [details] [review]
> patch
> 
> (In reply to comment #16)
> > Here is my current code, you should make a patch sharing the code to update
> > title position with updatePositions as you said.

Looks nice. I have been reading the patches manually and making changes to my 3.2.1 version, haha, so I have a little problem understanding the new _updateTitlePosition as I don't have the git version but if it works for you this bug seems resolved. :D

I downloaded the git version and patched it. Why do you pass an explicit false to _updateTitlePosition, from updatePositions? You should just pass the animate argument.

Also, from the notify::title function you pass true for animate. That should be a false. Otherwise you do some kind of _animateOverlayActor?

Created attachment 204439 [details] [review]
patch

(In reply to comment #19)
> I downloaded the git version and patched it. Why do you pass an explicit false
> to _updateTitlePosition, from updatePositions? You should just pass the animate
> argument.

this was a mistake. 

> 
> Also, from the notify::title function you pass true for animate. That should be
> a false. Otherwise you do some kind of _animateOverlayActor?

I think there's nothing wrong about true here, since it makes resize animated.

(In reply to comment #21)
> (In reply to comment #19)
> > I downloaded the git version and patched it. Why do you pass an explicit false
> > to _updateTitlePosition, from updatePositions? You should just pass the animate
> > argument.
> 
> this was a mistake. 
> 
> > 
> > Also, from the notify::title function you pass true for animate. That should be
> > a false. Otherwise you do some kind of _animateOverlayActor?
> 
> I think there's nothing wrong about true here, since it makes resize animated.

Okay, fix the mistake and upload again. Go for the animated change :D

    _animateOverlayActor: function(actor, x, y, width) {
        Tweener.addTween(actor,
                         { x: x,
                           y: y,
                           width: width,
                           time: Overview.ANIMATION_TIME,
                           transition: 'easeOutQuad'
                         });

Have you been testing an animated change? I know the unanimated looks good. It doesn't flicker or anything. Looks clean. If you know how the animation works go for animation otherwise pass false.

The animation works. It doesn't look mega-nice but it doesn't look that bad either. A nice little thing.

Created attachment 204440 [details] [review]
patch. animates title change

(In reply to comment #25)
> Created an attachment (id=204440) [details] [review]
> patch. animates title change

I had some troubles with animation testing at first, but now it looks fine. Seems like we've solved this bug.

(In reply to comment #26)
> (In reply to comment #25)
> > Created an attachment (id=204440) [details] [review] [details] [review]
> > patch. animates title change
> 
> I had some troubles with animation testing at first, but now it looks fine.
> Seems like we've solved this bug.

Yep :D How to get this reviewed and committed?

What do you think about the animation btw? I personally think it looks more elegant with no animation...

You should mark the faulty patch obsolete

(In reply to comment #28)
> What do you think about the animation btw? I personally think it looks more
> elegant with no animation...

Well, it's hard to say. No animation looks OK. I personally like it a bit more with animation.

In general, if there some users who don't like animation, it's better to go for no animation (I can't imagine anyone explicitly disliking no animation).

Small to large looks okay animated because everything is kept centred but I think large to small looks bad because the text is aligned to the left during the shrink so it's not centred during animation. No animation looks elegant either way.

Created attachment 204446 [details] [review]
style patch

(In reply to comment #31)
> Small to large looks okay animated because everything is kept centred but I
> think large to small looks bad because the text is aligned to the left during
> the shrink so it's not centred during animation. No animation looks elegant
> either way.

I see your point. 
We can add 
text-align: center;
to class
 .window-caption 
in
/data/theme/gnome-shell.css

I still think the animation looks bad. I would rather have the title just update. The important thing is to get this fix merged in time for 3.4. Someone needs to review and commit it. Let's see what others have to say about it.

(In reply to comment #20)
> Created an attachment (id=204439) [details] [review]
> patch

Review of attachment 204439 [details] [review]:

Vít and I are beginners but this patch seems to be all fine. It solves the reported bug and works great.

Created attachment 204548 [details] [review]
patch

This is the same patch but with proper git revision data as I missed that part earlier.

Created attachment 204570 [details] [review]
patch with _onTitleChange method

Since the function for updating title has grown a bit from its one line at the beginning, I've made it a new method. 
No change in functionality.

(In reply to comment #36)
> Created an attachment (id=204570) [details] [review]
> patch with _onTitleChange method
> 
> Since the function for updating title has grown a bit from its one line at the
> beginning, I've made it a new method. 
> No change in functionality.

Awesome! Can we have a patch and not a diff please?! [1]

[1] https://live.gnome.org/GnomeLove/SubmittingPatches

Created attachment 204680 [details] [review]
patch (not diff:-)

I really don't know where I learned that diff is patch. Anyway, thanks for enhancing my knowledge.

(In reply to comment #38)
> Created an attachment (id=204680) [details] [review]
> patch (not diff:-)
> 
> I really don't know where I learned that diff is patch. Anyway, thanks for
> enhancing my knowledge.

If I stay in the overview, the label width still does not get updated. Steps to reproduce:

 * Play an album in Rhythmbox, keeping the overview open

 * When the track changes, the window label updates but its width does not change to match the new content

 * Close the overview and open it again and the label is the redrawn with the correct width

(In reply to comment #38)
> Created an attachment (id=204680) [details] [review]
> patch (not diff:-)
> 
> I really don't know where I learned that diff is patch. Anyway, thanks for
> enhancing my knowledge.

You have done the same mistake again. You pass an explicit false to _updateTitlePosition and a true to animate from the _onTitleChange.

(In reply to comment #39)
> (In reply to comment #38)
> > Created an attachment (id=204680) [details] [review] [details] [review]
> > patch (not diff:-)
> > 
> > I really don't know where I learned that diff is patch. Anyway, thanks for
> > enhancing my knowledge.
> 
> If I stay in the overview, the label width still does not get updated. Steps to
> reproduce:
> 
>  * Play an album in Rhythmbox, keeping the overview open
> 
>  * When the track changes, the window label updates but its width does not
> change to match the new content
> 
>  * Close the overview and open it again and the label is the redrawn with the
> correct width

That's how I tested the patch. I have the 3.2.1 Shell however..

(In reply to comment #40)
> (In reply to comment #38)
> > Created an attachment (id=204680) [details] [review] [details] [review]
> > patch (not diff:-)
> > 
> > I really don't know where I learned that diff is patch. Anyway, thanks for
> > enhancing my knowledge.
> 
> You have done the same mistake again. You pass an explicit false to
> _updateTitlePosition and a true to animate from the _onTitleChange.

Thanks for noticing, The problem is, that I have more versions of the file. I
have to sort this out, get latest gnome shell for testing and it seems like a
lot to do. And if I try to make another patch, it contains only changes from
the previous one. 
I don't really feel like I have time for this today. Maybe at the weekend, but
I believe you will solve it until than.

Created attachment 204782 [details] [review]
Patch using PangoLayer to calculate new title width

Can you please try this patch instead? I found another way of determining new title width using the PangoLayer from this.title.clutter_text. I cannot test the git version currently but this works for my 3.2.1.

(In reply to comment #43)
> Created an attachment (id=204782) [details] [review]
> Patch using PangoLayer to calculate new title width
> 
> Can you please try this patch instead? I found another way of determining new
> title width using the PangoLayer from this.title.clutter_text. I cannot test
> the git version currently but this works for my 3.2.1.

I meant PangoLayout.. I'm sleepy :P

This isn't the final patch anyhow but I need feedback from someone with git version.

Created attachment 204790 [details] [review]
patch

Created attachment 204792 [details] [review]
This determines new title width by using theme properties and Pango

Created attachment 204806 [details] [review]
patch

This uses documented Pango functionality so I sure hope it works.

(In reply to comment #47)
> Created an attachment (id=204806) [details] [review]
> patch
> 
> This uses documented Pango functionality so I sure hope it works.

Does the job for me. :)

Great. I haven't found any issues using it. It works with large text and zoom.

Vít, can you mark "patch (not diff:-)" obsolete?

Review of attachment 204806 [details] [review]:

Overall looks fine, there are some style issues though.

The commit message could use some improvement too:

1. Wrap at 80 chars
2. Use something like "Overview: Resize the window title labels on content change"
3. Split the body in 2 sentences and don't write "Pango is used ..." but "Use pango ..."

::: js/ui/workspace.js
@@ +566,3 @@
     },
 
+    _onTitleChange: function(w) {

s/Change/Changed/

@@ +567,3 @@
 
+    _onTitleChange: function(w) {
+                let titlePadding = Math.ceil(this.title.width - this.title.clutter_text.

Indentation should be just 4 spaces.

@@ +568,3 @@
+    _onTitleChange: function(w) {
+                let titlePadding = Math.ceil(this.title.width - this.title.clutter_text.
+                                   get_layout().get_width() / Pango.SCALE);

Don't wrap like that "foo.bar.baz" should be on the same line IMO.

Created attachment 205320 [details] [review]
corrected patch

Oh crap.. titlePadding seems to be random when dragging a window to another workspace. I'll have to read the padding property instead. Sorry for this.

Review of attachment 205320 [details] [review]:

Marking as needs-work based on your comment.

The biggest problem I am facing is the fact that I really can't get gnome-shell git to compile/run. I actually spent like 12 hours of dependency solving yesterday without getting it to compile. I followed the gnome-shell jhbuild instructions and I was able to compile some programs like evince and gcalctool but the rest had too many unresolved dependencies even though I bootstrapped and installed like a thousand of extra packages.

What dist is preferable to get things done with ease? I use Fedora 16 currently. Can't even get rawhide to boot.

(In reply to comment #53)
> Oh crap.. titlePadding seems to be random when dragging a window to another
> workspace. I'll have to read the padding property instead. Sorry for this.

This is not a problem caused by this patch. I tried a fresh install of Fedora 16 with everything updated and vanilla (3.2.1) and I got the same bug. Someone should try patch id=205320 on gnome-shell git and see if it works.

Test if dragging a window to another workspace causes the title width to get a wrong size. If not, test if it does with patch id=205320 applied. If not, and everything seems well, commit. If test 1 and 2 is the same, this patch hasn't really changed that bug.

I need help with the testing. I have reinstalled my system and will try to compile gnome-shell git once again. My system was in pretty 'custom' shape last night :P

Created attachment 205598 [details]
this might be another bug

1. Enter overview
2. Launch at least two apps
3. Drag one window to the other workspace
4. Switch to that workspace

Is the title wrongly sized (a) without patch id=205320 (b) with patch id=205320?

This is what I get on a vanilla Fedora 16:

(In reply to comment #52)
> Created an attachment (id=205320) [details] [review]
> corrected patch

(In reply to comment #53)
> Oh crap.. titlePadding seems to be random when dragging a window to another
> workspace. I'll have to read the padding property instead. Sorry for this.

I finally got my gnome-shell git setup and tested this. I filed a separate bug because what I'm saying in comment 53 is not caused by patch id=205320 (comment 52).

https://bugzilla.gnome.org/show_bug.cgi?id=668241

If patch id=205320 looks okay it should be ready for commit.

I might need to fix something else I just found out. Bare with me..

(In reply to comment #59)
> I might need to fix something else I just found out. Bare with me..

If the text changes during a drag, the width is capped to the shrunken clone's width. It should cap to the width of the clone's final width (when not dragged).

+        let clone = this._windowClone;
+        let [cloneX, cloneY] = clone.actor.get_position();
+        let [cloneWidth, cloneHeight] = clone.actor.get_size();
+        cloneWidth *= clone.actor.scale_x;
+        cloneHeight *= clone.actor.scale_y;
+        this._updateTitlePosition(cloneX, cloneY, cloneWidth, cloneHeight, false);

This is not right because scale_x and scale_y is smaller during drag.

Created attachment 205696 [details] [review]
patch

I fixed the issue when text changes during drag. Apart from that, this patch really is the same as posted 7th January. There are some glitches left to solve but they are not part of this bug report. Please review and commit :)

Created attachment 205759 [details] [review]
patch

style fixed

Created attachment 205784 [details] [review]
patch

text change fixed:

* during initial window positioning
* during zoom

I can't find any other issues.

Created attachment 205787 [details] [review]
patch

I changed it to use theming and not delta width for padding. Delta width did in fact fail in some rare cases.

+        let clone = this._windowClone;
+        if (clone.inDrag || clone._zooming || this._hidden)
+            return;
+        this.show();

Could be 

+        if (!this._hidden)
+            this.show();

But I want feedback..

It doesn't work. I don't have any more time. I give up.

(In reply to comment #67)
> It doesn't work. I don't have any more time. I give up.

No problem Alex. Thanks for you work so far!

Review of attachment 205787 [details] [review]:

Marking needs-work as of comment #66

(In reply to comment #69)
> Marking needs-work as of comment #66

Comment #67 actually

Created attachment 226841 [details] [review]
Overview: Resize the window title labels on content change

Use pango to determine the width of the new text. Title padding is cached and
used together with the calculated text width to form the new title width. This
new title width is then applied every time a WindowOverlay shows.

Based on a patch by Alex Hultman <alexhultman@gmail.com>

(In reply to comment #71)
> Created an attachment (id=226841) [details] [review]
> Overview: Resize the window title labels on content change

Doesn't apply.

Created attachment 227182 [details] [review]
Overview: Resize the window title labels on content change

Use pango to determine the width of the new text. Title padding is cached and
used together with the calculated text width to form the new title width. This
new title width is then applied every time a WindowOverlay shows.

Based on a patch by Alex Hultman <alexhultman@gmail.com>

Rebased on the new window layout code.

Review of attachment 227182 [details] [review]:

The commit message is outdated. Code looks fine, though.

Created attachment 227400 [details] [review]
Overview: Resize the window title labels on content change

Reposition the window overlay when the title changes, using the current
transformed size of the window clone.
Includes a test that changes title to a string of random length every 3 seconds.

Based on a patch by Alex Hultman <alexhultman@gmail.com>

Here's a better one.

Review of attachment 227400 [details] [review]:

Looks good to me.

Attachment 227400 [details] pushed as 687e1ea - Overview: Resize the window title labels on content change