[pve-devel] [PATCH manager 4/5] Add icon for audio device

[Aaron Lauterer]

On 7/17/19 1:33 PM, Thomas Lamprecht wrote:
> On 7/16/19 5:12 PM, Aaron Lauterer wrote:
>> On 7/16/19 3:43 PM, Aaron Lauterer wrote:
>>> On 7/16/19 1:48 PM, Thomas Lamprecht wrote:
>>>> On 7/15/19 3:34 PM, Aaron Lauterer wrote:
>>>>>    www/images/icon-audio.svg | 2 ++
>>>> couldn't we just use existing font awesome icons, for example, "fa-volume-up"[0]
>>>> or eventual candidates could also, be "fa-headphones", "fa-music".
>>>
>>> This would indeed be a sensible thing to do. I looked into it and if I change the row configuration in `HardwareView.js` from
>>> "tdCls: 'pve-itype-icon-audio'," to "iconCls: 'volume-up'," it does show the icon using font-awesome nicely.
>>>
>>> The only downside is that the remove dialog shows the icon in the text:
>>>   > Are you sure you want to remove entry '<icon> Audio Device'
>>>
>>> This is because the remove dialog is calling the same `renderKey` function. My guess is that this has not been noticed yet as the `iconCls` property is used only by items that cannot be removed like BIOS or Memory. All the other hardware items use separate icons that are then handled with the `tdCls` property.
>>>
>>> How should I proceed as we most likely do not want to have the icon in the middle of the remove dialog?
>>>
>>
>> One possible way would be to set a property in the `metaData` object that is passed to the `renderKey` function to trigger a return value without the icon.
>>
> 
> I independently saw that the cloud icon for cloudinit was provided as SVG,
> and changed that, I come up with following solution and then checked your
> proposed. The fact that they are very similar in principle made me confident
> enough to straight apply it:
> 
> ----8<----
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 20f54cbe..94324323 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -33,7 +33,13 @@ Ext.define('PVE.qemu.HardwareView', {
>              icon = "<i class='pve-grid-fa fa fa-fw fa-" + iconCls + "'></i>";
>              metaData.tdCls += " pve-itype-fa";
>          }
> -       return icon + txt;
> +
> +       // only return icons in grid but not remove dialog
> +       if (rowIndex !== undefined) {
> +           return icon + txt;
> +       } else {
> +           return txt;
> +       }
>       },
>   
>       initComponent : function() {
> --
> 
> I like above a bit more as it needs no additional parameter and the icon
> really only makes sense in the grid view (where rowIndex is always defined).
> 
> Anyway, it should work now, thanks for the notice and highlighting the issue
> with already having a solution pronto!
> 

Definitely better! Thanks for quickly implementing it :)

>>
>> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
>> index 09d5d41f..8d58fd3d 100644
>> --- a/www/manager6/qemu/HardwareView.js
>> +++ b/www/manager6/qemu/HardwareView.js
>> @@ -33,6 +33,9 @@ Ext.define('PVE.qemu.HardwareView', {
>>              icon = "<i class='pve-grid-fa fa fa-fw fa-" + iconCls + "'></i>";
>>              metaData.tdCls += " pve-itype-fa";
>>          }
>> +       if (metaData.pveIcon === false) {
>> +           return txt;
>> +       }
>>          return icon + txt;
>>       },
>>
>> @@ -438,7 +439,7 @@ Ext.define('PVE.qemu.HardwareView', {
>>
>>                  var entry = rec.data.key;
>>                  var msg = Ext.String.format(warn, "'"
>> -                   + me.renderKey(entry, {}, rec) + "'");
>> +                   + me.renderKey(entry, {'pveIcon': false}, rec) + "'");
>>
>>                  if (entry.match(/^unused\d+$/)) {
>>                      msg += " " + gettext('This will permanently erase all data.');
>>
>