Created attachment 82541 [details]
My current idea of such a dialog.

Created attachment 82569 [details]
Fully functional implementation of the mockup.

I think the treeview may not be the best way to select font variants, also
while the dialog may look a bit more "modern", it really offers absolutely
no new functionality compared to the current dialog. I would be more interested 
I someone were to look into implementing some aspects of the more extensive
font dialog essay that somebody wrote a while ago. I don't have a pointer at hand, 
but I think it is rerferenced in some gtk+ too.

It was Ed Trager: http://unifont.org/fontdialog/

(In reply to comment #3)
> I think the treeview may not be the best way to select font variants

Well, got that idea from looking at the font collection program of MacOS 10.3 - liked it very much. Most fonts offer only a small number of font faces: Decorative (freeware) fonts mostly offer only one face. Quality fonts like the Bitstream's Vera fonts or Microsoft's web fonts offer four faces. Just some very few fonts have six faces, as they offer to choose between Italic and Oblique slanting. So in my opinion the faces list is a huge waste of space. The usefulness of the tree view is even more visible, when you use the search box to narrow the fonts.

> while the dialog may look a bit more "modern",

"It only just looks a bit more modern" is one of the worst excuses for improving something looking as horrible and out of place as the current font selection dialog.

> it really offers absolutely
> no new functionality compared to the current dialog. I would be more interested 

It offers some new functionality: The search box for narrowing the fonts shown. So for instance you can enter no "Bold Oblique" and only get the fonts offering this face. Tell me how you do that useful trick with the current font selection dialog.

> I someone were to look into implementing some aspects of the more extensive
> font dialog essay that somebody wrote a while ago. I don't have a pointer at
> hand, but I think it is rerferenced in some gtk+ too.

If you refer to the article Behdad mentioned: Looked at it and it enumerates several fauxpases found in current font selection methods and basically draws the conclusion: Have a look at the font dialog of MacOS - it gets it right. Accidently I did exactly that, as the font collection program of MacOS was my source of inspiration. The MacOS dialog itself is even more horrible than our current dialog. No idea what all the fancy buttons of that dialog do. But the font collection program is fine - expect for hidding the font size slider on the right side of the dialog. Didn't find it before opening the font collection program several times. 

A pretty feature of the font collection program I omitted is the collections list on the left, but don't know if Pango currently offers the information needed to build such a list. Ok, language specific groups ("Western Languages", "Chinese", "Japanese", "Arabic", could be formed by calling pango_font_get_coverage. "Monospace" also could be extracted. But groups like "Classic", "Modern", "PDF", "Fun" or "Web" would have to be setup manually - I guess. Do we want such a grouping feature in our font selection dialog?

Do we want to have the font installation feature of that dialog? Font installation is non-obvious on the GNOME desktop so far.

Well, and of course we have the problem of remaining binary compatible - especially as the current GtkFontSelection exposes far too much internal details...

Created attachment 82588 [details]
Font Collection program of MacOS 10.3

In one way is the treeview and the MacOS sample IMHO worse than the current font selector: to see the variants of a typeface one needs to expand the tree node (by clicking on a relatiely small triangle) (yeah, there probably is a keyboard shortcut for it), while in the current one sees them listed right away.

Where does MacOS get the information based on which it places some typefaces in the "Klassiker", "Moderne" and "Spass" ("Classics", "Modern" and "Fun" unlesss my German is rusty) categories? At first this sounds very sophisticated, but is it actually just the same crude enumeration which Win32 exposes as the FF_ROMAN, FF_MODERN, FF_DECORATIVE, FF_SWISS and FF_SCRIPT values in the LOGFONT::lfPitchAndFamily field, if somebody happens to know that? How useful is actually such a broad classification?

An ideal font selector would allow the user to introduce new classification, i.e. add new top-level categories and subcategories, and to move fonts between categories. Or even using keywords (tags) to describe fonts ;)

(In reply to comment #7)
> In one way is the treeview and the MacOS sample IMHO worse than the current
> font selector: to see the variants of a typeface one needs to expand the tree
> node (by clicking on a relatiely small triangle) (yeah, there probably is a
> keyboard shortcut for it), while in the current one sees them listed right
> away.

Well, it's easy to extend the code to expand on hover, selection or activate (double-click). Hover seems most conventient, but has some usability problems, like accidential expansion, when slowly navigating to the entry right below the automatically expanded entry - but maybe this can be addressed.

> Where does MacOS get the information based on which it places some typefaces in
> the "Klassiker", "Moderne" and "Spass" ("Classics", "Modern" and "Fun" unlesss
> my German is rusty) categories? At first this sounds very sophisticated, but is
> it actually just the same crude enumeration which Win32 exposes as the
> FF_ROMAN, FF_MODERN, FF_DECORATIVE, FF_SWISS and FF_SCRIPT values in the
> LOGFONT::lfPitchAndFamily field, if somebody happens to know that? How useful
> is actually such a broad classification?

At least some of the categories appears to be manual ("Web"), and MacOS allows to define your own categories.

> An ideal font selector would allow the user to introduce new classification,
> i.e. add new top-level categories and subcategories, and to move fonts between
> categories. Or even using keywords (tags) to describe fonts ;)

Tagging sounds interesting and also could be used by the search box... Just need some good UI for font tagging...

OK, if the MacOS selector allows defining new categories, that's nice.

BTW, note that the font selector in the development version of GIMP implements a little bit of the ideas in the article that Behdad linked to: For fonts that are mainly for some non-Latin script Instead of "Aa" it displays a similar sample from that script. It uses just heuristics of course, but seems to work fairly well in practise. (At least until some font turns up that contains well-designed glyphs for *both* Arabic and Thai, for instance.) See app/text/gimpfont.c:gimp_font_get_sample_string() in trunk gimp. See bug #137624. But still, it lists all fonts just in a big list and as such clearly isn't very usable with thousands of fonts.

Created attachment 82599 [details]
Mockup for a font selection dialog

I think my mockups belong here.

Created attachment 82600 [details]
another mockup with font size displayed

I think what's needed first is a combo box that displays the family names in
their respective faces. The categorization and additional information as proposed could be implemented next but needs a little infrastructure.

Just my 0.02€s

André(In reply to comment #12)
> I think what's needed first is a combo box that displays the family names in
> their respective faces. The categorization and additional information as
> proposed could be implemented next but needs a little infrastructure.

I and at least the author of Behad's article believe, that rendering font names using the font they describe looks fancy but has serious usability problems. Also I do not want to choose one of my 100+ fonts out of a combo box: It's too complicated. 

What I'd put into a combo box would be the category names: They are few, so a combo box might be acceptable. From looking at Tor's code (http://svn.gnome.org/viewcvs/gimp/trunk/app/text/gimpfont.c?view=markup) automatically categorizing fonts should not be that hard.

I really don't think a combo box is a good idea for the list of families, thats a long list, so the combo box is guaranteed to scroll, which is just inconvenient.

Regarding new features: Matthias, you are right, I hadn't noticed the filter which is in fact a useful new feature.

Regarding font installation: no, I don't think we want to integrate font installation in the font selection dialog. It is much more suitable to have it
a) more obviously in the font preference dialog 
and
b) in the context menu of font files

Regarding: API compatibility: if the old font selection dialog exposes internals,
and we are serious about including new features, it is probably better to make a clean cut and deprecate the old dialog, and add a new one, as we've done with the 
file selector. 

André, thanks for the mockups, but I agree with Matthias, we would really want something more hierarchic, in the style of the MacOS Font Collection screenshot, I think, with a user-editable hierarchical organisation of fonts. Also features usable when selecting fonts for multiple scripts, like that the sample string should be in the "main design script" of the font.

The sample string should perhaps not be in any real language like your "The quick brown fox", as that would then have to be translated. That would make little sense. If a user who uses an app in Arabic needs to write a bit of Russian, and chooses a font for that, why would she want to see the preview of that font in Arabic (which few of the fonts she would consider for Russian even cover)?

Anywy, just FYI, "The quick brown fox" is not a good sample string to use for displaying typeface features even for Latin script. One typically uses something that shows interesting kerning behaviour and ligatures. "Hamburgefons" is for some reason a traditional string often used for Latin scripts. (Or is it "Hamburgefonts"? Not "Hamburgerfonts" at least...) 

We don't want to go all the way and provide sample scripts in n different scripts each translated to m differerent languages that use that script. Or do we? Using "The quick  brown fox" as a sample text for a font for Latin when the UI is in English would mean that we should then use a similar phrase in German for a user who uses the app in German, right? Yeah, that would actually be neat. Hmm.

But would it make sense to translate "The quick brown fox" to Arabic or Chinese? Of course not, at least not if it is used to show a sample for a font for Latin. So this would need some very detailled instructions to the translators, and they would still not necessarily get it, and do pointless translations...

There would probably have to be explicit checks in the code: Call gettext() to translate the sample string for the script the font seems to be designed for only if that script is the one used by the current UI language. Otherwise use the original sample string for that script from the source code.

Another issue is that fonts often contain localized or transcribed names. We should probably show the native Chinese name of a font for Chinese at least when the user's locale is Chinese (or Japanese or Korean?) But the latinized name should still be accepted too in places like gtkrc files or pango.aliases.

Fonts can contain localized names? Interesting!

Regarding the sample text: I really had in mind to translate it. A good sample text for German language is: "Zwölf Boxkämpfer jagen Viktor quer über den großen Sylter Deich..." - contains all interesting chars. Cann't the sample text be choosen to match the currently selected category? If "Chinese" is choosen as category a chinese sample text is shown, if no language specific category is choosen, first the font is checked to properly support the current locale, otherwise your heuristic is used? Does the localized font name provide some information for choosing a good sample text? If it is Unicode and mostly uses Arabic codepoints it might be some good indication, that this font is designed for arabic usage? How many localized names Truetype fonts can contain? Only one? Or several for different languages?

Ok, you asked for it!

I think a new object makes sense.  And there's a lot to borrow from filechooser.

Combo box is awefully bad.  Forget it.

Having style in separate box (or combo) is better IMO.  I've wanted to fix the current dialog to remember the chosen style when you select a different face.  That is not possible with a tree.  And I prefer the clean cut of faces and styles in the current setup.

For font size, a slider is probably fine, but make sure it doesn't limit the size (at least not to something silly like 100).

As for sample text, an editable combobox is the way to go.  In fact, it should be the same box that you show the font.  Not a seprate box.  And the combo box contains some good pangrams and other useful text for the language. (http://en.wikipedia.org/wiki/Pangram).  We use the translation mechanism, but make it very clear to translators that these should be translated to a pangram in the target language.  I know it's really hard to make sure translators understand that.  Another option is to use keywords like "pangram1" "pangram2" ... for translation key.

Font installation would be really cool from here.  It's like New Folder in a file dialog.  But we don't have that these days, so maybe not.

For showing face names using their font or not, both cases are really useful.  I think a checkbox may be all we need.  Otherwise, a two column list. (loading all fonts may be slow and consume lots of memory, so would be better to not load them all by default).

About collections, I've had the idea for some time, but we need some new API in fontconfig.  Something like "list collections".  The idea is to move font configuration from XSETTINGS to fontconfig config.  When we do get that support, we can handle collections like Places in file chooser.

'nough for now :)

I have to disagree about using pangrams. Firstly, such only exist for "trivial" restricted alphabets such as used by English or Swahili. For most languages (even those using Latin scripts), desperately looking for a pangram will just be an exercise in futility. Secondly, the point in viewing a sample text in the font preview is not so much that you would want to be certain to see *every* letter. It's just important to see letters that show typical features, and (hopefully) letter pairs where the kerning is important. See http://desktoppub.about.com/od/typedesign/f/Hamburgefonts.htm 

(In reply to comment #18)
> I have to disagree about using pangrams. Firstly, such only exist for "trivial"
> restricted alphabets such as used by English or Swahili. For most languages
> (even those using Latin scripts), desperately looking for a pangram will just
> be an exercise in futility. Secondly, the point in viewing a sample text in the
> font preview is not so much that you would want to be certain to see *every*
> letter. It's just important to see letters that show typical features, and
> (hopefully) letter pairs where the kerning is important. See
> http://desktoppub.about.com/od/typedesign/f/Hamburgefonts.htm 

Right.  That doesn't change much though.  Replace pangram with "sample text" in my comment.

Created attachment 84757 [details]
Mockup with tagging support

This variant adds a text entry for tagging. Source code is available at
http://taschenorakel.de/svn/repos/font-dialog-ng/trunk/

Wondering how I could give the size and the tags padding some more padding on the left without using nasty tricks.

Created attachment 84758 [details]
English version of the previous mockup

I was looking for another bug, but here are my thoughts on this one:
* Tree navigation seems very good.
* Whatever "sample text" you provide by default, I would appreciate it to be possible to edit in the dialog. Perhaps the phrase could be remembered?
Examples:
 - when having a specific word to select special font for.
 - include special characters of my own choice, to quickly notice if the font lacks them or makes them ugly. Just selecting sample text based on my UI language might not always be sufficient.

If my comments already are implemented, then please ignore me! I couldn't think of a way to launch the current "hopelessly outdated" dialog and test it. But from Gedit's Preferences dialog, in the editing font-tab, I found a dialog that allowed the sample phrase to be customized.

Created attachment 117213 [details]
An alternative mockup

There is an ongoing discussion at http://aruiz.typepad.com/siliconisland/2008/08/font-selectio-1.html and I think it should be moved here.

I've attached an alternative proposal. I think we can merge preview with the style selection widget. We should also expose a GObject property to let applications provide their own preview text (defaulting to the lazy fox) so we can:

1) see the result of applying the font on the selected text instead of some arbitrary string

2) propose a list of fonts that actually contain all the selected glyphs (or glyphs for the language of selection if such information is available)

In future one could also provide a custom preview cell renderer so for example GNOME's "window title font" selector draws a fake window border around the text etc.

I'd also love to see grouping (see the arrow to the left of the filter box on my mockup) by specific features such as "fonts free for commercial use," "fonts that are free to embed" and other open font license details (also filtering to show just TTF fonts for example).

*** Bug 548167 has been marked as a duplicate of this bug. ***

I'm working with the design team in coming up with an actual replacement of the font dialog. The work lives i this[0] github branch.

Currently I'm working on not breaking API, thus being able to return the treeviews from get_size_list/family_list/face_list even though they do not exist anymore. I'm marking those as deprecated anyhow.

I'd like to talk about this work on the next Gtk+ meeting and propose it for 3.2

[0] - https://github.com/aruiz/gtk-fontselector-branch

Is Mairin part of the team?  We met about this a couple years ago and she took some detailed notes...

Mairin helped me with the original designs, though lately it has been mostly Allan Day, Hylke Bons and Lapo Calamandrei.

Humm.  I really love to give this a run, but I'm facing generic build issues with gtk right now.  I'll prolly give it another try next week or so.  Feel free to kick me later.  Thanks for working on this!

Some initial comments after playing with this for a few minutes:

- The way the list scrolls on selection feels wrong to me. I see what you are trying to do here, but I don't like it. If I click on a font towards the bottom of the currently visible set, it jumps to the middle. Very disorienting.

- The search entry should probably follow GNOME3 style and use symbolic icons.

- I think the preview entry needs to grow vertically to accomodate big font sizes, instead of growing a scrollbar.

- You need to somehow avoid fallbacks in the preview text rendering, otherwise it is just confusing. I pasted ♆♛♜⚪ into the entry, and it happily showed those for characters in exactly the same style all the time as I scrolled through the fonts, none of which had those characters...

- Not sure the current filtering is good enough. I found that I could see only italic fonts by typing 'italic' in the entry, but then how do I filter further ? adding another word after italic left me with only hershey fonts, since those are the only ones which have 'italic ' in the middle of their name. Maybe break the search entry text into words and filter by those words individually ?

- The gray font names are ok in general, but in the selected row, the name should be made white, otherwise the contrast is not good enough.

- two good proposals that came up in mairin's old work, which aren't implemented here yet:

  * Have placeholder text for the list when there are no matching fonts:
    its somewhere in https://live.gnome.org/Design/GTKFontDialog/MockupSet1

  * Display coverage information by graying out fonts that lack some of the
    characters in the preview text

Oh, and 0 should not be an allowed font size.

Quick look at the code:

- Your use of deprecation guards is wrong. They are a header-only feature, you cannot use them to ifdef out parts of the implementation. 

- We should take this occasion and properly split gtkfontsel.[hc] into
  gtkfontselection.[hc] and gtkfontselectiondialog.[hc]

- The old font selection API was always a bit small; we should consider adding
  e.g. application-provided font filtering, or a range of acceptable sizes.
  Typical use case would be to only show fixed-width fonts.


> * Since: 3.2: Use gtk_font_selection_dialog_get_select_button instead.

That function does not exist. And it probably shouldn't; you can just use gtk_dialog_get_widget_for_response()

- It looks like your deprecated internal-widget-getters are not actually compatible. They used to return treeviews, but yours returns scrolled windows.

Some more comments:

- To fix the scrolling behaviour I complained about, you want to change all your scroll_to_cell calls to pass FALSE for use_align

- The internal structure should ported from obsolescent h/vboxes to a grid, and the fontsel itself should be moved to be a bin child, probably

Hello Mathias,

Thanks a lot for having such a detailed look at the branch.

Agreed with all your comments, will work on them during this week, I'll be moving the branch as soon as I fix a little git mess (I've pulled without rebasing a few times) :-)

My only concern is regarding the preview entry, do we want it to grow dinamyically?

My understanding is that windows that change their size is a bad thing in general, in this particular case the problem is that the size controls will move away from the the pointer if you use the scale to resize. I am concerned about the scrollbar too though, I hope you don't mind if I try to gather some feedback about this particular issue with the design guys as I don't think this is just a matter of getting rid of the scrolled window.

Other than that, really good catches! I really appreciate the feedback.

(In reply to comment #34\
> 
> My only concern is regarding the preview entry, do we want it to grow
> dinamyically?
> 
> My understanding is that windows that change their size is a bad thing in
> general, in this particular case the problem is that the size controls will
> move away from the the pointer if you use the scale to resize. I am concerned
> about the scrollbar too though, I hope you don't mind if I try to gather some
> feedback about this particular issue with the design guys as I don't think this
> is just a matter of getting rid of the scrolled window.

No, I don't think we want the window to grow; we want the font list to shrink (while keeping the selected row visible).

I've just pushed some fixes I've worked on in the last few days which include:

• Symbolic icon for the search entry
• Fix scroll to selection alignment
• Split filter text in terms
• Fix theming of the font name color (standard foreground for now, will improve with a data_func)
• Split gtkfontsel into gtkfontselection and gtkfontselectiondialog
• Remove GTK_DISABLE_DEPRECATED from .c
• Deprecate all button getters and remove get_select_button

I'm working on the remaining issues, will update as I fix/push.

Created attachment 187582 [details]
Font dialog horizontal layout

Made some adjustments to the layout so we don't need to scale any widgets on the fly. It has all the features as Aruiz's implemenatation + the selected font name above the preview and a unit string after de spinbutton.

I think this would work pretty well. The only bad thing about it is that a horizontal scrollbar may appear. However, this isn't as bad as a vertical one (a rare occasion, because seeing the complete height of the glyphs is more important than the whole sentence. A vertical scrollbar should never appear in this design.

(In reply to comment #36)
> I've just pushed some fixes I've worked on in the last few days which include:
> 
> • Symbolic icon for the search entry
> • Fix scroll to selection alignment
> • Split filter text in terms
> • Fix theming of the font name color (standard foreground for now, will improve
> with a data_func)
> • Split gtkfontsel into gtkfontselection and gtkfontselectiondialog
> • Remove GTK_DISABLE_DEPRECATED from .c
> • Deprecate all button getters and remove get_select_button

These look all great to me, thanks.

While playing with the current branch, I get segfaults out of _pango_engine_shape_shape when scrolling quickly up and down. That may not be your fault, though...

I also get warnings about

(lt-testfontselectiondialog:30745): GLib-GObject-WARNING **: cannot create instance of abstract (non-instantiatable) type `PangoFallbackEngine

I'll have a look into those issues as soon as I get out of the GtkFontChooser move work, I'm reworking the branch to preserve history while moving the code to another widget, will publish the new branch (font-chooser) sometime today.

Interested to know what's going on with pango.  You're not offloading work to threads, are you?

(In reply to comment #39)
> I also get warnings about
> 
> (lt-testfontselectiondialog:30745): GLib-GObject-WARNING **: cannot create
> instance of abstract (non-instantiatable) type `PangoFallbackEngine

This is bogus.  Suggests memory corruption of some sort going on IMO.  Valgrind.

No threads whatsoever, no. As far as I know, I'm doing a straightforward use of the API.

I just managed to get a backtrace of the issue:

_pango_engine_shape_shape (engine=0x0, font=0x9883e0, text=0xaa2400 "●", length=3, analysis=0xcd4d50, 
    glyphs=0x7e82e0) at pango-engine.c:94
94	  PANGO_ENGINE_SHAPE_GET_CLASS (engine)->script_shape (engine,
(gdb) bt

+ Trace 227088

Regardless, why is it trying to shape bullet?  You don't have any password entry there I assume.

Look at gtkentry.c:find_invisible_char

it is interested in      

 count = pango_layout_get_unknown_glyphs_count (layout);

for each of the possible invisible chars, and the bullet is one of them.
Perhaps there should be a more direct way to ask 'is this character covered by the current fonts ?'

(In reply to comment #46)
> Look at gtkentry.c:find_invisible_char
> 
> it is interested in      
> 
>  count = pango_layout_get_unknown_glyphs_count (layout);
> 
> for each of the possible invisible chars, and the bullet is one of them.
> Perhaps there should be a more direct way to ask 'is this character covered by
> the current fonts ?'

I meant: find_invisible_char() should be lazier.

For the record,

I have created a new "font-chooser" branch with the new widget and removed the "font-selector-new" one. This branch is using the GtkGrid widget instead of V/HBox, given the rebase magic I had to do to preserve the history of gtkfontsel.c there might be some regressions, I've been working on it but I've had a tight week.

Here's an update:

My work was stalled due to my home workstation's hard drive suicidal tendencies, I've been short on budget so took me a while to replace. Sorry about this.

The GtkGrid port is completed, the layout works as it should as per last discussions in the Gtk+ meeting. The preview grows as the scrolled window shrinks.

I have also removed the greyed out text as it was broken theming wise. I'll leave it with the default color as getting it right might mean having to write a more complicated data function or even a custom CellRenderer. Suggestions are welcome here though.

I think that the current work is good enough for 3.2, there are some improvements to be made moving forward, but I'd like some input as to what's essential for merge and 3.2 readyness.

Thanks, Alberto.

I will spend some time with it this weekend.

Overall this looks really good, and quite close to good enough.
Some quick comments:

Behaviour:

- There seems to be a slight disconnect between the size slider and spin button: 
  the slider is only updated for every second change in the spin button, when
  using keynav.

Appearance:

- Not sure if the current padding-less grouping of search entry, list and preview
  entry is what the designers asked for. It looks a bit unfortunate to me.

- The initial size of the dialog seems too small. It should come up large enough
  to show at least 3 lines in the list, and not have the slider marks merge into
  one solid block, at least.

- When there are no matches for the search text, should we show some text like
  'Sorry, no matches found' ?

API:

- GtkWidget *gtk_font_chooser_dialog_new (const gchar *title)

I think we need to have a parent argument here, as we do for other dialogs.


- GtkWidget* gtk_font_chooser_dialog_get_font_selection (GtkFontChooserDialog *fcd);

Should perhaps be gtk_font_chooser_dialog_get_font_chooser, given that it is
not returning a GtkFontSelection.

- We should either just port GtkFontButton to always use the new dialog, or
  add a way to switch between old and new. Probably the former.

Implementation:

#define MAX_FONT_SIZE 999

Is that reasonable ?

#define FONT_SIZES_LENGTH 14

Much better to use G_N_ELEMENTS() instead of an extra define like that.


    case PROP_SHOW_PREVIEW_ENTRY:
      gtk_font_chooser_set_show_preview_entry (fontchooser, g_value_get_boolean (value));
    default:

Missing break here

    case PROP_SHOW_PREVIEW_ENTRY:
      g_value_set_boolean (value, gtk_font_chooser_get_show_preview_entry (fontchooser));
    default:

and here


void
refilter_and_focus (GtkFontChooserPrivate *priv)

Missing static here


void
deleted_text_cb (GtkEntryBuffer *buffer,

and here


void
inserted_text_cb (GtkEntryBuffer *buffer,

and here ... and many more


  if (g_strcmp0 (gtk_entry_get_icon_stock (GTK_ENTRY (entry), GTK_ENTRY_ICON_SECONDARY),
                 "edit-clear-symbolic") ||
      g_strcmp0 (gtk_entry_get_icon_stock (GTK_ENTRY (entry), GTK_ENTRY_ICON_SECONDARY),
                 GTK_STOCK_CLEAR))

I prefer explicit comparisons for anything involving strcmp; ie
g_strcmp0 (...) != 0 


  if (priv->ignore_slider)
  {
    priv->ignore_slider = FALSE;
    return;
  }

Wrong indentation here


  for (i=0; i<length; i++)

Lack of spaces around = and < here


  if (!face || !family)
    {
      g_object_unref (face);
      g_object_unref (family);
      return;
    }

This looks bad; if one of them is NULL, you unref them both, which guarantees
you an ugly warning from g_object_unref (NULL); also, I prefer == NULL over !
for pointers.


  g_signal_connect (G_OBJECT (gtk_entry_get_buffer (GTK_ENTRY (priv->search_entry))),
                    "deleted-text", G_CALLBACK (deleted_text_cb), priv);

This looks suspicious; why do you go to the buffer here, instead of
simply using GtkEntry signals ? I don't think there's any need for it.


  g_signal_connect (G_OBJECT (priv->preview), "scroll-event",
                    G_CALLBACK (zoom_preview_cb),  priv);


As a matter of style, I would normally prefer to pass the widget instead
of its private.


gtk_font_chooser_ref_family
gtk_font_chooser_ref_face

These are really confusingly named, in particular since you end up
calling ref functions in the finalize(). They are really just setters.


 * Note that the @fontchooser needs to know the screen in which it will appear 
 * for this to work; this can be guaranteed by simply making sure that the 
 * @fontchooser is inserted in a toplevel window before you call this function.

I don't think this is a reasonable restriction (but I don't think it is
really true either).


 fontchooserdiag = g_object_new (GTK_TYPE_FONT_CHOOSER_DIALOG, NULL);

  if (title)
    gtk_window_set_title (GTK_WINDOW (fontchooserdiag), title);

I wonder why we do things like this here. I would just write

  dialog = g_object_new (GTK_TYPE_FONT_CHOOSER_DIALOG, "title", title, NULL);

nowadays. Also, unfortunate choice of variable name here.



  if (g_strcmp0 (childname, "select_button") == 0)
    return G_OBJECT (priv->select_button);
  else if (g_strcmp0 (childname, "cancel_button") == 0)
    return G_OBJECT (priv->cancel_button);
  else if (g_strcmp0 (childname, "font_chooser") == 0)
    return G_OBJECT (priv->fontchooser);

Is there any reason for exposing the buttons to GtkBuilder ? I can maybe see
the point for the font_chooser.


Trivia:

 * GtkFontChooser widget for Gtk+, by Damon Chaplin, May 1998.
 * Based on the GnomeFontSelector widget, by Elliot Lee, but major changes.
 * The GnomeFontSelector was derived from app/text_tool.c in the GIMP.

Should perhaps ditch some of this, since it is really not true for this new
widget...


/*****************************************************************************
 * GtkFontChooserDialog functions.
 *   most of these functions simply call the corresponding function in the
 *   GtkFontChooser.
 *****************************************************************************/

Not font of this kind of decoration in headers.


There's some formatting issues in the code, such as trailing whitespace and
not quite perfectly lined up parameters, such as:

static void 
gtk_font_chooser_set_property (GObject         *object,
                                 guint            prop_id,
                                 const GValue    *value,
                                 GParamSpec      *pspec)

or here

void
set_range_marks (GtkFontChooserPrivate *priv,
                 GtkWidget* size_slider,
                 gint* sizes,
                 gint length)

or here

gboolean
zoom_preview_cb (GtkWidget *scrolled_window, GdkEventScroll *event, gpointer data)

(In reply to comment #51)
> Overall this looks really good, and quite close to good enough.
> Some quick comments:
> 
> Behaviour:
> 
> - There seems to be a slight disconnect between the size slider and spin
> button: 
>   the slider is only updated for every second change in the spin button, when
>   using keynav.

Fixed

> 
> Appearance:
> 
> - Not sure if the current padding-less grouping of search entry, list and
> preview
>   entry is what the designers asked for. It looks a bit unfortunate to me.

Indeed, this was a regression I mishandled during the GtkGrid port. I tried with gtk_grid_set_row_spacing but that didn't have any effect (not sure if this is a bug, if so, let me know and I'll report it), I'm manually setting the margin-bottom on each element.

Fixed.
  
> API:
> 
> - GtkWidget *gtk_font_chooser_dialog_new (const gchar *title)
> 
> I think we need to have a parent argument here, as we do for other dialogs.

Fixed

> - GtkWidget* gtk_font_chooser_dialog_get_font_selection (GtkFontChooserDialog
> *fcd);
> 
> Should perhaps be gtk_font_chooser_dialog_get_font_chooser, given that it is
> not returning a GtkFontSelection.

This function actually didn't exist, it was fixed in the .c and not in the .h.
Fixed.

>     case PROP_SHOW_PREVIEW_ENTRY:
>       gtk_font_chooser_set_show_preview_entry (fontchooser, g_value_get_boolean
> (value));
>     default:
> 
> Missing break here
> 
>     case PROP_SHOW_PREVIEW_ENTRY:
>       g_value_set_boolean (value, gtk_font_chooser_get_show_preview_entry
> (fontchooser));
>     default:
> 
> and here

Fixed

> 
> 
> void
> refilter_and_focus (GtkFontChooserPrivate *priv)
> 
> Missing static here
> 
> 
> void
> deleted_text_cb (GtkEntryBuffer *buffer,
> 
> and here
> 
> 
> void
> inserted_text_cb (GtkEntryBuffer *buffer,
> 
> and here ... and many more

Fixed.

>
>   if (g_strcmp0 (gtk_entry_get_icon_stock (GTK_ENTRY (entry),
> GTK_ENTRY_ICON_SECONDARY),
>                  "edit-clear-symbolic") ||
>       g_strcmp0 (gtk_entry_get_icon_stock (GTK_ENTRY (entry),
> GTK_ENTRY_ICON_SECONDARY),
>                  GTK_STOCK_CLEAR))
> 
> I prefer explicit comparisons for anything involving strcmp; ie
> g_strcmp0 (...) != 0 

Fixed.

> 
> 
>   if (priv->ignore_slider)
>   {
>     priv->ignore_slider = FALSE;
>     return;
>   }
> 
> Wrong indentation here

This snippet doesn't exist any more as per fix of the size sync fix above. I took care that the new code had proper indentation.

Fixed.
 
>   for (i=0; i<length; i++)
> 
> Lack of spaces around = and < here

Same style issue in various places.
Fixed.
 
>   g_signal_connect (G_OBJECT (priv->preview), "scroll-event",
>                     G_CALLBACK (zoom_preview_cb),  priv);
> 
> 
> As a matter of style, I would normally prefer to pass the widget instead
> of its private.

Fixed

>  fontchooserdiag = g_object_new (GTK_TYPE_FONT_CHOOSER_DIALOG, NULL);
> 
>   if (title)
>     gtk_window_set_title (GTK_WINDOW (fontchooserdiag), title);
> 
> I wonder why we do things like this here. I would just write
> 
>   dialog = g_object_new (GTK_TYPE_FONT_CHOOSER_DIALOG, "title", title, NULL);
> 
> nowadays. Also, unfortunate choice of variable name here.

Fixed.

> 
> Trivia:
> 
>  * GtkFontChooser widget for Gtk+, by Damon Chaplin, May 1998.
>  * Based on the GnomeFontSelector widget, by Elliot Lee, but major changes.
>  * The GnomeFontSelector was derived from app/text_tool.c in the GIMP.
> 
> Should perhaps ditch some of this, since it is really not true for this new
> widget...

Fixed.

> /*****************************************************************************
>  * GtkFontChooserDialog functions.
>  *   most of these functions simply call the corresponding function in the
>  *   GtkFontChooser.
>  *****************************************************************************/
> 
> Not font of this kind of decoration in headers.
> 

Fixed

> There's some formatting issues in the code, such as trailing whitespace and
> not quite perfectly lined up parameters, such as:
> 
> static void 
> gtk_font_chooser_set_property (GObject         *object,
>                                  guint            prop_id,
>                                  const GValue    *value,
>                                  GParamSpec      *pspec)
> 
> or here
> 
> void
> set_range_marks (GtkFontChooserPrivate *priv,
>                  GtkWidget* size_slider,
>                  gint* sizes,
>                  gint length)
> 
> or here
> 
> gboolean
> zoom_preview_cb (GtkWidget *scrolled_window, GdkEventScroll *event, gpointer
> data)

Fixed

> #define FONT_SIZES_LENGTH 14
> Much better to use G_N_ELEMENTS() instead of an extra define like that.

Fixed.

--- Request for comments:

>   if (g_strcmp0 (childname, "select_button") == 0)
>     return G_OBJECT (priv->select_button);
>   else if (g_strcmp0 (childname, "cancel_button") == 0)
>     return G_OBJECT (priv->cancel_button);
>   else if (g_strcmp0 (childname, "font_chooser") == 0)
>     return G_OBJECT (priv->fontchooser);
> 
> Is there any reason for exposing the buttons to GtkBuilder ? I can maybe see
> the point for the font_chooser.

Maybe there isn't I was just blindly following the implementation of the fontselector. I can remove that if you think we should.

>   g_signal_connect (G_OBJECT (gtk_entry_get_buffer (GTK_ENTRY
> (priv->search_entry))),
>                     "deleted-text", G_CALLBACK (deleted_text_cb), priv);
> 
> This looks suspicious; why do you go to the buffer here, instead of
> simply using GtkEntry signals ? I don't think there's any need for it.

None of the entry signals seemed right to figure out when text is inserted or deleted. I could use "notify" on the "text" property I guess but I can't hardly see how is that is an improvement. In any case I can still change it to do that if you still think is the right way to move forward.

> - When there are no matches for the search text, should we show some text like
>   'Sorry, no matches found' ?

I think we should, but I don't have any Gtk-realistic designs. I will poke the design guys and see if they can come up with something. I don't want to just shove a GtkLabel to replace the ScrolledWindow when the model is empty.

--- ToDo (this is really to help me track what's left from the original comments):

> - The initial size of the dialog seems too small. It should come up large
> enough
>   to show at least 3 lines in the list, and not have the slider marks merge
> into
>   one solid block, at least.
 
> - We should either just port GtkFontButton to always use the new dialog, or
>   add a way to switch between old and new. Probably the former.

> #define MAX_FONT_SIZE 999
> Is that reasonable ?
 
>   if (!face || !family)
>     {
>       g_object_unref (face);
>       g_object_unref (family);
>       return;
>     }
> 
> This looks bad; if one of them is NULL, you unref them both, which guarantees
> you an ugly warning from g_object_unref (NULL); also, I prefer == NULL over !
> for pointers.

> gtk_font_chooser_ref_family
> gtk_font_chooser_ref_face
> 
> These are really confusingly named, in particular since you end up
> calling ref functions in the finalize(). They are really just setters.

>  * Note that the @fontchooser needs to know the screen in which it will appear 
>  * for this to work; this can be guaranteed by simply making sure that the 
>  * @fontchooser is inserted in a toplevel window before you call this function.
> 
> I don't think this is a reasonable restriction (but I don't think it is
> really true either).

> - We should either just port GtkFontButton to always use the new dialog, or
>   add a way to switch between old and new. Probably the former.

It is always using the new one now. The old one is deprecated as per this new widget so we shouldn't use it.

Fixed.

> - The initial size of the dialog seems too small. It should come up large
> enough
>   to show at least 3 lines in the list, and not have the slider marks merge
> into
>   one solid block, at least.

Fixed.

> #define MAX_FONT_SIZE 999
> Is that reasonable ?

Ah, I forgot to put this in the RFC section. My rationale behind that is that I wanted to bound the size of the spinbutton to the minimum possible. 999 seemed a number absurd enough to me. Obviously, if you think that bigger sizes are plausible, we can go for 9999 or 99999. I'm open to suggestions.

(In reply to comment #52)


> 
> --- Request for comments:
> 
> >   if (g_strcmp0 (childname, "select_button") == 0)
> >     return G_OBJECT (priv->select_button);
> >   else if (g_strcmp0 (childname, "cancel_button") == 0)
> >     return G_OBJECT (priv->cancel_button);
> >   else if (g_strcmp0 (childname, "font_chooser") == 0)
> >     return G_OBJECT (priv->fontchooser);
> > 
> > Is there any reason for exposing the buttons to GtkBuilder ? I can maybe see
> > the point for the font_chooser.
> 
> Maybe there isn't I was just blindly following the implementation of the
> fontselector. I can remove that if you think we should.

Not a big deal. Can just leave it in

> >   g_signal_connect (G_OBJECT (gtk_entry_get_buffer (GTK_ENTRY
> > (priv->search_entry))),
> >                     "deleted-text", G_CALLBACK (deleted_text_cb), priv);
> > 
> > This looks suspicious; why do you go to the buffer here, instead of
> > simply using GtkEntry signals ? I don't think there's any need for it.
> 
> None of the entry signals seemed right to figure out when text is inserted or
> deleted. I could use "notify" on the "text" property I guess but I can't hardly
> see how is that is an improvement. In any case I can still change it to do that
> if you still think is the right way to move forward.

I would prefer to just use notify::text here.


I'd be happy to defer the 'No matches found' label until after the initial merge.

I've committed a number of changes to the branch tonight, including:

>   if (!face || !family)
>     {
>       g_object_unref (face);
>       g_object_unref (family);
>       return;
>     }
> 
> This looks bad; if one of them is NULL, you unref them both, which guarantees
> you an ugly warning from g_object_unref (NULL); also, I prefer == NULL over !
> for pointers.

> gtk_font_chooser_ref_family
> gtk_font_chooser_ref_face
>

Things left to do from my perspective:

- Initial size

- Address "Note that the @fontchooser needs to know the screen in which"

- 'No matches'

Merged for 3.1.12

http://l10n.gnome.org/module/gtk+/ lists several files related to this as forgotten to have been added to POTFILES.in.