bndtools / bnd

Bnd/Bndtools. Tooling to build OSGi bundles including Eclipse, Maven, and Gradle plugins.
https://bndtools.org
Other
526 stars 304 forks source link

Hide <<EMPTY>> repo tag in Repo Browser #6171

Open chrisrueger opened 2 days ago

chrisrueger commented 2 days ago

Based on suggestion by @juergen-albert in https://github.com/bndtools/bnd/pull/6170#issuecomment-2200178047

Why this PR?

The screenshot in the other PR looked like this:

image

The <<EMPTY>> in the repo browser might be misleading, as one could think "the repo is empty".

To better reflect this intention we should find another way.

In this PR

We keep the <<EMPTY>> placeholder, but we display it differently in the repo browser.

We now do this:

image
pkriens commented 2 days ago

I think there is a misunderstanding. The <> value signals no value. I.e. as it it wasn't set. It should not be visible in the UI since it should be treated as if the property was not set.

In bnd we often have inheritance of variables, making it impossible to 'unset' a value. I.e. if you remove the property, the super value pops up. If you set it to <>, it is treated as if it wasn't set.

chrisrueger commented 2 days ago

It should not be visible in the UI since it should be treated as if the property was not set.

Ahhh ok. So when you say "UI": Which places do you have in mind?

The repo browser (where I just introduced the display of the tags and may have caused the issue of misinterpretation)

image

What would you display instead when <<EMPTY>> was set?

Somewhere else?

pkriens commented 2 days ago

Anywhere you use the value, treat <> is if it had never been set.

chrisrueger commented 2 days ago

Suggestion:

image
pkriens commented 2 days ago

yup!

chrisrueger commented 2 days ago

I pushed another approach updated the PR description. Basically it is now:

image

See also the contained testcase.

@juergen-albert does it improve things?

juergen-albert commented 1 day ago

That is definitely better. Just to be sure: If no tag is set, the repo is used for resolving as well, same as if resolve is set?

chrisrueger commented 1 day ago

That is definitely better. Just to be sure: If no tag is set, the repo is used for resolving as well, same as if resolve is set?

Yes

juergen-albert commented 1 day ago

I don't want to be a pain in the ass here, but I'm still a bit unhappy with the -. If somebody like my colleagues update to the new version they will start seeing the - behind every Repository, without any indication what this means or what the consequences are.

Right now, only repositories with no tags or with the tag resolve will be used which is implicit knowledge. Thus I suggest, we either show nothing, if no tags are set or something that indicates the resulting behavior like resolve by tag default or something like that.

Showing nothing hides the tags until somebody stumbles upon this and hopefully starts reading the documentation before experimenting with it. The other version, indicates that there are now tags and that they come with additional functionalities attached.

chrisrueger commented 1 day ago

I don't want to be a pain in the ass here, but I'm still a bit unhappy with the -.

The discussion is totally fine and appreciated.

If somebody like my colleagues update to the new version they will start seeing the - behind every Repository, without any indication what this means or what the consequences are.

No. They will see "resolve" behind every repository. The - is rather the exception for once special case. See below.

Right now, only repositories with no tags or with the tag resolve will be used which is implicit knowledge. Thus I suggest, we either show nothing, if no tags are set or something that indicates the resulting behavior like resolve by tag default or something like that.

At the moment the logic is as follows:

Case 1:

Case 2:

Case 3:

Case 4:

Case 5:

Showing nothing hides the tags until somebody stumbles upon this and hopefully starts reading the documentation before experimenting with it. The other version, indicates that there are now tags and that they come with additional functionalities attached.

To summarize what changes in the new version, as of the current state:

juergen-albert commented 1 day ago

Ahh, thanks heaps for the explanation. I totally misread the code and your previous statements. I never realized, that resolve is the actual default if no tags are set!

In this case: +1 from me.

chrisrueger commented 9 hours ago

Then this is ready to merge. I would do this later today if there are no further objections.