Thanks, I can confirm this.

Created attachment 71286 [details]
First draft of a patch to fix this

In my patch, I've added a new "IsLinkRole", similar to "IsDirRole", to KFileItemModel, and let KFileItemListWidget::paint() set the font to italic if that role is true for the widget's data.

An alternative would be to not change paint(), but to change the widget's style option in KFileItemListWidget::dataChanged() if the item is a link. But I see a risk that the style option change could get overwritten if setStyleOption() is called from somewhere else.

In any case, the patch in its current state does not apply to master due to the changes in KFileItemListWidget/KStandardItemListWidget. I see two ways to change this at the moment:

1) Change the syle option in KFileItemListWidget::dataChanged(), as I said above.

2) Add a new virtual isLink() method to KStandardItemWidget, similar to isHidden(), and reimplement that method in KFileItemListWidget. This looks like the cleaner solution to me.

Thanks Frank for the initial patch!

I'm not sure whether adding a "isLink"-method to KStandardItemListWidget is a valid usecase for KStandardItems...

But I must admit that the whole split between KItemListWidget < KStandardItemListWidget < KFileItemListWidget is somehow a "grey area": Moving generic stuff into the base-class makes it more comfortable for derived classes, but the definition of "generic" depends a lot on the usecase and of course moving everything into the base-class is also a no-go :-(

In the very longterm the KItemListWidget and the derived classes should be replaced by QML, but we'll have to do the same decisions there.

Hm, in any case changing the style is not the correct approach (although each item instance has its own style instance, the basic idea was that the style itself is common for all items) - I'd say lets go for your suggestion and add a protected "isLink"-method to KStandardItem with a comment "TODO: Check whether this is a a generic enough usecase to have it as part of KStandardItem".

After having implemented all the missing functionality we might check again the protected methods of KStandardItems: e.g. there are currently custom properties like "text-color" and "overlay" vs. virtual getters for isHidden(), isRightAligned() - both approaches have their pros and cons.

So as summary: Please feel free to make it work first with the "isLink()" approach. If we later on come to the conclusion that the protected virtual getters should be replaced by something else, we can change this.

Created attachment 71308 [details]
Second draft of a patch (for master)

Thanks for the comments, Peter! I was also unsure if adding the virtual isLink() method to KStandardItemListWidget makes sense, but I thought that it's basically comparable to isHidden(), and moreover, not doing it that way would require more changes in KFileItemListWidget.

It seems to work OK with my patch applied to master, but I'm still not 100% happy with it. The approach I've used so far is to make the font italic in paint() based on the isLink-setting, but there are some other methods of KStandardItemListWidget that use the style option's font and fontMetrics for height and width calculations. The metrics of the italic font might be different from the non-italic one, so I'm wondering if this might lead to problems (I remember some subtle and hard-to-find bugs which were due to incorrect width calculations in KDE <= 4.7). These could be resolved by changing the style, but I agree that this does not feel quite right.

> The metrics of the italic font might be different from the non-italic one,
> so I'm wondering if this might lead to problems (I remember some subtle
> and hard-to-find bugs which were due to incorrect width calculations in
> KDE <= 4.7). 

Ah - yes: I remember this kind of issues too now, so just painting it in italic is not sufficient (as you said already).

What do you think about the following approach:
- Add a private member QFont m_customizedFont to KStandardItem
- Create a protected virtual method QFont KStandardItem::customizedFont(const QFont& baseFont) const;
- We call this method internally in KStandardItem::onStyleOptionChanged() and do a:
  m_customizedFont = customizedFont(current.font);
- Internally in KStandardItem we alway uses m_customizedFont instead of styleOption().font.
- Per defaul KStandardItem::customizedFont(...) will just return baseFont.

So in the end all we'd have to do is to overwrite customizedFont() in KFileItemListWidget. 

One open issue: We'd need to invalidate the customized font in case if the "isLink" property would change... Hm, probably we should just introduce a protected setCustomizedFont()/customizedFont()? I need to go offline now - let me think about this a little bit and lets continue the discussion later ;-)

Your idea looks good!

I think you mean KStandardItemListWidget instead of KStandardItem, right?

> One open issue: We'd need to invalidate the customized font in case if the
> "isLink" property would change... Hm, probably we should just introduce a
> protected setCustomizedFont()/customizedFont()?

Couldn't we just do "m_customizedFont = customizedFont(current.font)" in KStandardItemListWidget::triggerCacheRefreshing()? In that case, the customized font would be updated both after style option changes and after data changes.

(In reply to comment #6)
> I think you mean KStandardItemListWidget instead of KStandardItem, right?

Yes :-)

> Couldn't we just do "m_customizedFont = customizedFont(current.font)" in
> KStandardItemListWidget::triggerCacheRefreshing()? In that case, the
> customized font would be updated both after style option changes and after
> data changes.

You are right, that should work! This is what I meant also in comment 3 with:
> After having implemented all the missing functionality we might check again
> the protected methods of KStandardItems: e.g. there are currently custom
> properties like "text-color" and "overlay" vs. virtual getters for isHidden(),
> isRightAligned() - both approaches have their pros and cons.

From a maintenance point of view I'd prefer having only one approach. To me the virtual getters-method + having an internal cache in the base-class seems to be the better approach. The setter + getter-approach is a little bit more flexible and (in theory) allows to do a more optimized refreshing of the cache - but it increases the risk to miss a cache-update... It would be great if you could share your opinion on this (must not be in the context of this bug - it is more about having a stable, consistent solution in the longterm).

Sorry for the late reply. I also think that virtual getters + internal caches in KStandardItemListWidget are the better approach. Your argument that it reduces the risk of missing cache-updates is very convincing IMHO.

I'll update my patch based on your m_customizedFont + virtual QFont customizedFont() idea.

Created attachment 71363 [details]
New patch for master, using new members m_customizedFont and m_customizedFont in KStandardItemListWidget

Thanks Frank for the patch - it looks very good, please go ahead with committing it to master :-) When reading the bug-report initially I thought this should be only a few lines of code, but I think your solution is fine and we are prepared now for further wishes like this.

Git commit fe8ef7776c7ba80a96d587b5310067c62cc90b13 by Frank Reininghaus.
Committed on 26/05/2012 at 15:32.
Pushed by freininghaus into branch 'master'.

Use an italic font for symbolic links
FIXED-IN: 4.9.0

M  +1    -0    dolphin/src/kitemviews/kfileitemlistview.cpp
M  +8    -0    dolphin/src/kitemviews/kfileitemlistwidget.cpp
M  +1    -0    dolphin/src/kitemviews/kfileitemlistwidget.h
M  +9    -0    dolphin/src/kitemviews/kfileitemmodel.cpp
M  +1    -1    dolphin/src/kitemviews/kfileitemmodel.h
M  +29   -20   dolphin/src/kitemviews/kstandarditemlistwidget.cpp
M  +7    -0    dolphin/src/kitemviews/kstandarditemlistwidget.h
M  +2    -1    dolphin/src/tests/kfileitemmodeltest.cpp

http://commits.kde.org/kde-baseapps/fe8ef7776c7ba80a96d587b5310067c62cc90b13