ni / labview-icon-editor

Source for LabVIEW's icon editor
MIT License
15 stars 11 forks source link

Automatically detect mismatched banners on VIs part of LVLIBs #40

Open svelderrainruiz opened 1 month ago

svelderrainruiz commented 1 month ago

This issue stems from a comment contained on https://github.com/ni/labview-icon-editor/issues/19

Quote: _Optional feature extension: During its loading/start-up procedure the Icon Editor could detect if the VI/CTL has the "wrong" banner (banner does not match current owner banner). If that is the case, the Icon Editor could display a two button dialogue containing a message such as:

"This VI does not use the banner of its owner, "name of owner here". Would you like the banner to be applied?"

If the user selects "Yes" (or hits Enter), the Icon Editor would execute the "Import Icon from Owning Library..." action for the user.

Benefits:

This functionality would act like a mini VI analyser. It would help catch this issue. Clicking Yes (or hitting Enter) would be even quicker than executing the keyboard shortcut. Downsides: Performing the check may slow down the start-up sequence. This may be a significant drawback.

Also enable a checkbox to "Always Apply". _

svelderrainruiz commented 1 month ago
image

I am thinking on having 2 checkboxes for this.

stagg54 commented 1 month ago

sounds reasonable.

crossrulz commented 1 month ago

I think it would nice to have the Icon Editor detect if the owning library's icon was updated and put a warning in a window status text or similar. This would be not in your face and force you to update the icon. To remove the notice, one just needs to update the NI_Library layer via the menu or shortcut.

svelderrainruiz commented 1 month ago

@gregr-ni @dnattinger do you believe this is a change that can be brought back into the product if i do it?

gregr-ni commented 1 month ago

The goal is to have the shipping icon editor be built from github, so everything that happens here should be pulled back into the product. We don't have that process automated yet though.

svelderrainruiz commented 1 month ago

These are the 4 functions that i am going to develop:

1 - Enable automatic banner mismatch detection:

image

2 - Auto import icon from owning library:

image

image

3 - Auto save VI when importing owning library:

image

image

4 - VIs with any of the following criteria will not be able to use this feature, by automatically disabling it for the course of the lifespan of that icon editor execution

chrisb2244 commented 1 month ago

Hi - a few thoughts/comments:

crossrulz commented 1 month ago
* Although it's a different direction to the suggestion here, and I'm not particularly opposed to your proposed change, have you considered instead of a 2 button dialog on open using a separate button to "Apply Class/Library Icon" or similar (probably on the layer page?). This would I think solve the same problem and perhaps not involve too many more clicks (i.e. the same number maybe), but I guess the focus in the modal would make hitting Enter faster...

There is already a menu item to apply the library icon and @svelderrainruiz added a shortcut for that item.

Again, I don't like the dialog. A warning status text would be sufficient, in my opinion.

svelderrainruiz commented 1 month ago

Hi - a few thoughts/comments:

> * What do you mean by "VI is listed as a dependency" (project dependency? I guess to avoid changing code that isn't part of the current project/focused area?)

Correct, project dependency. This feature would only work for VIs that are part of a project. This is to avoid accidentally triggering a change on a VI that may already been modified previous to opening the icon editor

_> * Please don't detect a difference if the layer is the same but hidden - I don't want to be repeatedly prompted just because it's not visible or something (not sure how you plan to code this, or if detecting similarity would be affected by visibility toggles, but just in case) (Likewise, do you plan to address at all the layer height, or just leave it where it is? I guess if there is no NILibrary layer, it would go on top, but if there is a different layer, it would be replaced in the same 'height'?)

Noted, i will create a unit test that will be able to catch that

_> * If possible, a way to say no and not be repeatedly prompted would be nice, but stashing extra info into the VI (e.g. via tags) might be a pain for SCC and potentially confuse other developers who didn't set the 'remember no' tag and then didn't expect a 'wrong' icon without a prompt. Not sure what I'd suggest here, but repeated prompts for the same VI is probably annoying (though maybe pretty niche, if you're detecting specifically a layer titled "NILibrary" like the current implementation).

This is something i would definitely use... ill add this

  • Although it's a different direction to the suggestion here, and I'm not particularly opposed to your proposed change, have you considered instead of a 2 button dialog on open using a separate button to "Apply Class/Library Icon" or similar (probably on the layer page?). This would I think solve the same problem and perhaps not involve too many more clicks (i.e. the same number maybe), but I guess the focus in the modal would make hitting Enter faster...

As @crossrulz mentioned, i made a new feature that is pending approval from @gregr-ni that allows you to Import owning library icon to current VI by pressing CTRL+I

svelderrainruiz commented 1 month ago
* Although it's a different direction to the suggestion here, and I'm not particularly opposed to your proposed change, have you considered instead of a 2 button dialog on open using a separate button to "Apply Class/Library Icon" or similar (probably on the layer page?). This would I think solve the same problem and perhaps not involve too many more clicks (i.e. the same number maybe), but I guess the focus in the modal would make hitting Enter faster...

There is already a menu item to apply the library icon and @svelderrainruiz added a shortcut for that item.

Again, I don't like the dialog. A warning status text would be sufficient, in my opinion.

@crossrulz , enabling this option would skip the dialog, and would give you only a warning on the icon editor if the icon editor detects a mismatch between NI_Library layer from the VI Icon and the icon from the lvlib.

image