|Summary:||Rework alertpanel focus handling|
Description flo.xfce 2017-04-27 23:35:29 CEST
Created attachment 1747 [details] Rework alertpanel focus handling The current way of giving focus to a button other than button1 in the alertpanel we prepend a plus sign to the label. This is bad style. Also, this breaks building with the --enable-gtk3 flag (which is unsupported atm, true, but still another reason to rework the current code). The attached patch introduces a seperate flag to specify which button in the alertpanel should be focused. A bit more typing is neccessary, but yields much cleaner code. The patch is rather big because there are so many calls to alertpanel(), but the changes are trivial. The patch was created with git diff origin/master..alertpanel_rework -- . ':!po'
Comment 1 flo.xfce 2017-05-10 18:23:54 CEST
I thought about this some more. My intention was to ease porting efforts to gtk3. However, in gtk3, stock items, which are heavily used in alertpanels will be deprecated. Those stock items bundle a label and an icon name. Thus, if we at one point in time port away from stock items we would have to rework the alertpanel code again to take both the button labels and the button icons as seperate parameters. I really don't want to go through the whole rework more than once, better make it right the first time. Passing icon names and button labels seperately, together with the focus parameter introduced in this bug report, brings the number of parameters in alertpanel_create to 13, which I consider bad coding style. Any ideas how to proceed? Take the discussion to the mailing list or here in bugzilla?
Comment 2 Michael Rasmussen 2017-05-11 01:18:52 CEST
Perhaps pass a struct containing all parameters as parameter to alertpanel()?
Comment 3 Andrej Kacian 2017-05-11 10:02:08 CEST
Please do not make a simple API into an unmaintainable and unusable nightmare like this. Surely GTK3 has support for freedesktop icon names, which can be used instead of deprecated GTK stock icons? https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
Comment 4 flo.xfce 2017-05-11 12:16:48 CEST
Passing a struct will not really help. Look at the patch to get an idea of how many call to alertpanel_* there are. Everytime we would have to fill a new struct, this does not help readability or maintainability at all. Of course gtk3 has support for freedesktop icon names, but it deprecated the mapping between an icon and a label. gtk_button_new_from_stock (which is used internally in alertpanel_create) just needs the stock item and infers label and icon automatically, this is no longer possible with non-deprecated api. gtk_button_new_from_icon_name and gtk_button_set_label do this now.
Comment 5 Andrej Kacian 2019-01-03 00:49:33 CET
This has been fixed already, no more "+" prefix for focus in alertpanel API. See https://git.claws-mail.org/?p=claws.git;a=commitdiff;h=5ab9e6