Show Maven artifacts via list instead of tabs for the dependency editor. #1157
Conversation
|
Before |
|
@ptziegler if you really like to go with the table approach, would it not be more suitable to go this path all way down and have an editable table with five rows? This would then also allow having sorting by clicking the header by group/artifact/version/.... wdyt? |
I assume you mean columns and not rows? It should make the list entries (GAV+Type) a lot more readable. Switching from a ListViewer to a TableViewer shouldn't be difficult, so I'll give it a go and see how it looks. |
And then have the cells being editable items: https://www.vogella.com/tutorials/EclipseJFaceTable/article.html#column-editing-support |
|
@laeubi I think you're right. Having everything in a simple table looks a lot cleaner. And being able to sort by column can be quite convenient, I think :)
|
|
This looks really promising, I just think we now have a lot of free "gray area", do you maybe like to try out:
Beside that, maybe a "update to latest version" button would be useful, we already have this functionality in the editor, but it seems a good place to have it here as well! |
|
That's a very nice improvement. 👍🏽 |
|
I've moved the buttons to the right side and also added an Update button, which looks up the latest versions of the selected artifacts. Though I ended up dropping the idea of using both images and text. For some reason, they kept being centered and therefore none of the images lined up properly. Which didn't look very good. From what I found, this seems like a very old limitation in SWT (bug 172850). On the bright side, the new layout looks similar to the Target editor, which is probably a good thing. |
There was a problem hiding this comment.
I think the UI looks very good now some remarks on the code:
- There are a lot of whitespace/formating only changes, it would be good to reduce these to make review easier (e.g. only formating changed lines)
- Some comments on the code itself below
- You are using
PropertyChangeListenersto update the UI, probably usingIObservablesandISideEffectFactorywould be more convenient than handling this manually. - You could add a not (and your last screenshot) to the RELEASE_NOTES
| import org.eclipse.swt.dnd.TextTransfer; | ||
| import org.eclipse.swt.widgets.Display; | ||
|
|
||
| public abstract class ClipboardUtils { |
There was a problem hiding this comment.
Can this static method be moved to ClipboardParser instead of having a util class?
| * Undo context used by m2e for all internal modifications of the Maven | ||
| * dependency table (e.g. the initialization). | ||
| */ | ||
| public static IUndoContext SYSTEM_CONTEXT = new UndoContext(); |
There was a problem hiding this comment.
If it is used in the dependency table only, maybe these are better suited to be places there?
| * </ul> | ||
| * </p> | ||
| */ | ||
| public class MavenAccessor implements Function<Object, String>, BiConsumer<Object, Object> { |
There was a problem hiding this comment.
Object seems not really they type to use here but MavenTargetDependency / String?
| public final class MavenTextEditingSupport extends EditingSupport { | ||
| private final MavenAccessor accessor; | ||
|
|
||
| public MavenTextEditingSupport(ColumnViewer viewer, MavenAccessor accessor) { |
There was a problem hiding this comment.
Probably it would be better instead of having MavenAccessor to pass the getter/setter here to remove an indirection?
There was a problem hiding this comment.
Having this indirection was the initial idea. In the end, I removed this class completely in favor of directly passing on the getter/setter.
|
|
||
| @Override | ||
| protected Object getValue(Object element) { | ||
| return accessor.apply(element); |
There was a problem hiding this comment.
here would be a better place to cast the object than in the MavenAccessor
|
|
||
| @Override | ||
| protected Object getValue(Object element) { | ||
| String value = accessor.apply(element); |
There was a problem hiding this comment.
Do we really need to operate on the index? What about the user is entering a custom type (what is allowed)
|
|
||
| @Override | ||
| public void widgetSelected(SelectionEvent e) { | ||
| sortDirection = sortDirection != SWT.UP ? SWT.UP : SWT.DOWN; |
There was a problem hiding this comment.
sortDirection would probably better be a boolean and then map it to the integer afterwards? Using != here makes this line looking more complex as it is.
|
|
||
| @Override | ||
| public String getText(Object element) { | ||
| return getter.apply((Object) element); |
| public class MavenTargetColumnLabelProvider extends ColumnLabelProvider { | ||
| private Function<Object, String> getter; | ||
|
|
||
| public MavenTargetColumnLabelProvider(Function<Object, String> getter) { |
There was a problem hiding this comment.
I think this should better be a parmeterized class.
| /** | ||
| * <p> | ||
| * This class provides the ability to edit the type of a Maven dependency via a | ||
| * combo box. Supported types are: |
There was a problem hiding this comment.
This is wrong, the combo-box should be editable one so the user can type in any string/type these are only some common types we want to offer the user a choice without typing it in manually.
ptziegler
force-pushed
the
newTargetDependencyEditor
branch
from
d1cd7ab to
083faad
Compare
Yeah... the auto-formatter is both a blessing and a curse. I think it only affected a single class. My apologies. But I think I've resolved all of the issues. I've split up the UI from the "model" via the ISideEffect and IObservable API and so far everything seems to be still working. |
|
Hm... I ran an RCPTT test earlier today and it seems like it's getting stuck in an infinite loop when accessing the IObservable containing the IOperationHistory instance. |
|
When I recently used RCPTT it fails to work with the target editor ... if you was able to get it working it would be great to share the RCPTT test! Does this loop also happen in a regular eclipse? If not it might be good to report a bug to RCPTT ... |
I already planned on doing that once I'm happy with the test case.😅 But I think this should be done via a separate pull request. Because I need to include the RCPTT runner in the build job. Which isn't really difficult with the maven-rcptt-plugin, but unrelated to the dependency editor.
I'm not exactly sure... The UI is responsive in both the regular Eclipse and in RCPTT. So I assume the update of the IObservable is done in a background task. And because the runner is waiting for the Eclipse to become "ready", it gets stuck indefinitely. It's entirely possible that the same happens during the normal operation, it just isn't noticable. Then again... I ran into some sporadic OutOfMemory/Heap Space errors during the past few days where I couldn't really track down the root cause. There is a good chance that those are related, if the update function is called indefinitely while the editor is open. |
|
I could try to take a look at the code a bit closer today ... but in general you can try to set a breakpoint in the update function to see if it triggers again and again. Sadly there are some operations in the observable framework of eclipse that are not very optimal and one can easily create infinite updates for some collection operations. e.g. if you clear a collection (that already is empty) it is still considered a change... |
At least for the RCPTT tests, that's what I did. And also how I figured out why it got stuck. I'll test it again this evening with the regular Eclipse.
I don't think it's an issue with the Observable framework, but rather my custom implementation. My idea was that the Undo/Redo buttons should be updated, whenever a new operation is added or removed from the IOperationHistory instance. However, the DefaultOperationHistory I'm using (which is sadly the only implementation) stores the operation in normal lists instead of IObservableLists. Meaning the side-effect mechanism doesn't work here. I wrote the OperationHistoryObservableValue class to work around this limitation but I guess it doesn't work quite as well as I had hoped. |
|
|
||
| @Override | ||
| protected T doGetValue() { | ||
| fireEvent(new ChangeEvent(this)); |
There was a problem hiding this comment.
this seems not correct here why is getting a value firing a change event?
There was a problem hiding this comment.
That's what I meant... It's a quick & dirty solution to make sure the buttons are updated, whenever something is done to the IOperationHistory instance.
The Undo button, for example, checks its enablement state by calling IOperationHistory.canUndo(IUndoContext).
But because this call doesn't access any IObservables, the side-effect for the button is never executed again. My "fix" was to create a custom dummy IObservableValue wrapping the IOperationHistory. which notifies this side-effect, whenever accessed.
There was a problem hiding this comment.
You should not call the IOperationHistory directly but for example OperationHistoryFacade#canUndo (like outlined below) that then delegates to the IOperationHistory
Can you say where it get infinite loop?
Usually with such types it is the best to have a facade that manages the access to the underlying object that extends |
Not now, but I'll get back to you once I'm back home. I assume it's caused by the
Thanks for the suggestion! I'll try that out later today. |
ptziegler
force-pushed
the
newTargetDependencyEditor
branch
from
083faad to
101c2e8
Compare
|
Your recommendation was spot-on @laeubi. |
There was a problem hiding this comment.
The result of this change looks very nice.
But I have the impression that at multiple places the code could be more compact. While it is good to separate concerns, distribution code over more classes makes it also harder to understand because one has to navigate more classes. Of course this has to be weighted but I think in this case multiple classes can be inlined and replaced by lambdas or anonymous classes, without making the other class too large. I made some suggestions in the comments below.
Since this contribution is currently greater than 1000 lines I wonder if we still have to create a Contribution Questionary to the IP team?
...de.ui/src/org/eclipse/m2e/pde/ui/target/editor/internal/part/DependencyEditorViewerPart.java
Outdated
Show resolved
Hide resolved
| * Updates the input and selection of this viewer with the entries from the | ||
| * model. Does nothing if the entries are already up-to-date. | ||
| */ | ||
| private void onInputChange() { |
There was a problem hiding this comment.
This method and the following onSelectionChange() seem to be simple enough to be inlined, which would replace the method-reference by a lambda-block.
| * be updated with the new value but the input and selection should not be | ||
| * updated explicitly, as this is already handled by JFace. | ||
| */ | ||
| public class ModifyPropertyOperation extends AbstractOperation { |
There was a problem hiding this comment.
This class is also only used once and could be replaced by a corresponding anonymous class at the caller.
There was a problem hiding this comment.
If it helps to keep the code more focused I think a "extra" class is fine, but it could probably made package private to show it is only used here.
| * location, as well as the list of all currently selected elements. This change | ||
| * operation triggers all registered side-effects over those attributes. | ||
| */ | ||
| public class ModifyModelOperation extends AbstractOperation { |
There was a problem hiding this comment.
This class is also only used once and could be replaced by a corresponding anonymous class at the caller.
There was a problem hiding this comment.
I'm not sure if inlining the class makes it more clear, but it coud be package private
There was a problem hiding this comment.
Even though there might be some improvements e.g. in the update handling I think the overall change is fine, I will open a CQ just t be sure.
@ptziegler if you like just proceed with @HannesWell to improve some code places and you might want to take a look at the WidgetProperties and the ViewerProperties class, they allow to create observable values e.g. for a selection that the can be used inside side-effects to synchronize state between model/ui in a littel more convenient ways than registering listeners manually.
|
The handbock says:
But I can't find it on the project page https://projects.eclipse.org/projects/technology.m2e @HannesWell any idea? |
|
@HannesWell the reported p2 baseline problem is it related to: |
HannesWell
force-pushed
the
newTargetDependencyEditor
branch
from
101c2e8 to
8d415c8
Compare
|
Furthermore eventually this PR should be submitted as one commit. if you want you can squash all commits into one giving it a meaning full header and message body that covers the result (no need to capture each step in that) or we will squash-and-merge commit it and try to extract a good message. |
Empty rows should be ignored, but I won't put much requirements on incomplete ones as we did not enforced this before and as the user can edit the xml anyways it does not make much sense to be over pendantic here.
I don't think this is a good idea as it will result in massive overhead and might produce a lot of garbage in the local repository, resolving is actually part of the target resolution and not a concern of the editor. |
Just "ignoring" them feels wrong. And I think it can lead to unexpected behavior when you, for example, simply forgot to add the version. Or in a case where we would check whether the artifact exists, the user made a typo or used the wrong type. I'm leaning towards disabling the "Finish" button + Error message, as you suggested.
Depending on how complicated this is, I can probably do it as part of the validation. Though I'm a little bit concerned about the performance implications, if I talk to Maven Central, every time I add/edit a Maven artifact. But that can probably only be seen when it's actually implemented.
So should it then still be possible to sort by Artifact Id/Version or only by Group Id? In that case I would simply sort by MavenTargetDependency#getKey(), which I think is what's used when the target location is saved.
Absolutely. I'll probably use my first commit message with a little bit more detail. But at least while developing, I like to have an overview over the individual changes. |
Yes, the ignore part was from my initial version of the message, where I just covered empty row. As Christoph said, I think it is fine to ignore completely empty ones, but for incomplete ones an error+ disabled finish is better.
Even if that did not exist why not improve that now? IMHO that would fit quite well and should not be too much work.
Yes a full resolution would indeed be out of scope. Do you know if it is possible to only check if a artifact exists in the local or any of the available remote repositories, without resolving/downloading it? That would at least harm the performance much less than a full resolution with a potential download.
That's a good question. I mean the sorting in general has no real meaning and it just changes the displayed order in this editor. So in most cases it is probably irrelevant. I can think it could be useful if you for example search for a certain artifact and don't know the group (or similar). So I would not object if we keep the per column sorting, but I also don't think it is critical to have. So I leave that decision to you :)
If that isn't a string that's fine 👍🏽 If it is a string we should check if the result is as desired with the separator.
Sure that's fine and reasonable. Just wanted to make you aware. :) |
You can try issuing a version request: But this just keep in mind that this ca open a can of worms, just assume you specify additional repositories, these must then re-resolve all items (as now it might be there), then what happens if it does not work because of wrong maven settings so we need to probably offer a link to open/change the maven settings... What could work quite out of the box would be to have a But lets finish this before ;-) |
That's not the only issue. The table is initialized with the original target location. Which also means it would use the original repositories. If the dependency can't be found on e.g. Maven Central, you would then have to construct some meta target location containing both the original and newly added (or removed) repositories, pass it to the do the whole thing again and who knows what else...
That sounds a lot like what the target editor is doing... |
ptziegler
force-pushed
the
newTargetDependencyEditor
branch
from
6c648ab to
89f2180
Compare
|
I've made some improvements based on your feedback.
|
|
@ptziegler looks great and thanks for all the effort, just a really minor remark: |
ptziegler
force-pushed
the
newTargetDependencyEditor
branch
from
89f2180 to
b53d3dd
Compare
Simple enough. I just had to set the margin to 0. It didn't really bother me, given that both the top and bottom had the same margin, but I don't mind either way. |
There was a problem hiding this comment.
Looks great now! Your suggestions are perfectly fine.
Just a few minor suggestion below, but they are totally optional.
I also noticed, when playing around with it a bit that the minimum sizes of the columns as well as of the entire editor could be improved.
The column width, at least up until a certain extend, could be fitted to the content.
And it should not be possible to make the entire dialog smaller than the (radio) buttons in the lower part.
But I'm fine with doing that later.
...ipse.m2e.pde.ui/src/org/eclipse/m2e/pde/ui/target/editor/internal/TargetDependencyModel.java
Outdated
Show resolved
Hide resolved
...ipse.m2e.pde.ui/src/org/eclipse/m2e/pde/ui/target/editor/internal/TargetDependencyModel.java
Outdated
Show resolved
Hide resolved
...ipse.m2e.pde.ui/src/org/eclipse/m2e/pde/ui/target/editor/internal/TargetDependencyModel.java
Outdated
Show resolved
Hide resolved
...ipse.m2e.pde.ui/src/org/eclipse/m2e/pde/ui/target/editor/internal/TargetDependencyModel.java
Show resolved
Hide resolved
Table widths are always a funny thing to deal with... How it currently works is that if you open the editor for the first time, the widths are adjusted so that none of the entries are obstructed. Though this might still happen, if you create new elements. I've adjusted it so that the column width is also adjusted, when the input is modified.
There I would go all the way. I calculate the preferred size of the shell when opening the editor and set that as the minimum size. You shouldn't be able to make the shell so small that you can't modify the target definition anymore. |
ptziegler
force-pushed
the
newTargetDependencyEditor
branch
from
b53d3dd to
48e8254
Compare
|
I think we should just set a minimum size for the dialog, and then probably use the table column layout? That reminds me of that I always wanted to contribute a more advanced layout for table columns than whats currently available in SWT... |
But the wizard dialog is created by the target editor, so we don't have direct access to it. You also don't have a minimum size in e.g. the p2 wizard. Perhaps this is something that should be brought up to the PDE team? |
|
One should be able to call IWizard#getContainer().getShell() and might resize the shell if it is too small when there is a request to show the page, but I have not tried that out. |
I think this would better be JFace/Platform UI, it would be good if a IWizardPage would have a getMinimumPageSize and getMaximumPageSize (the default can just use getControl().computeSize(-1,-1)) |
I've created eclipse-platform/eclipse.platform.ui#516. It's probably for the best to postpone this topic until there's a response from the Platform UI team. |
ptziegler
force-pushed
the
newTargetDependencyEditor
branch
from
48e8254 to
9d51c01
Compare
There was a problem hiding this comment.
Table widths are always a funny thing to deal with...
UI is a funny thing and tables are especially funny, yes. :D
I think this would better be JFace/Platform UI, it would be good if a IWizardPage would have a getMinimumPageSize and getMaximumPageSize (the default can just use getControl().computeSize(-1,-1))
I've created eclipse-platform/eclipse.platform.ui#516. It's probably for the best to postpone this topic until there's a response from the Platform UI team.
Yes, lets delay that.
I'm fine with this change as it is right now.
@laeubi are you as well?
@mickaelistria can you tell if we still have to create CQs for contributions over 1000LOC? Because I at least know that the term CQ has been deprecated, but I don't know if we have to create something else instead?
org.eclipse.m2e.pde.ui/src/org/eclipse/m2e/pde/ui/target/editor/internal/DependencyTable.java
Outdated
Show resolved
Hide resolved
The current implementation can only show ~5 items at a time. All remaining dependencies have to be selected via a combo box. For projects with several dozen dependencies, it is often faster to edit the XML file directly, rather than using this wizard. The new implementation keeps all dependencies in a table. Items can be sorted by column and arbitrary elements can be added or removed. Support has been added to update a selected number of artifacts to their latest version. Undo/Redo functionality has been added. A simple validation is added to check that all required attributes of a dependency are set. Meaning group id, artifact id, version and type.
HannesWell
force-pushed
the
newTargetDependencyEditor
branch
from
73808e6 to
1a20be4
Compare
I checked the Eclipse projects handbook and reviews for contributions over 1000LOC of non-Committers are still necessary. |
|
I see a few hundreds lines that are "just" moving code or extracting methods, so from IP perspective, those lines are not really counting. If we take the new production, the new content for which IP maters, we're under 1000 lines. So I think we could skip the CQ and this would still be fine. |
Alright. Thanks for that clarification. Additionally each new file contains 12 lines of copyright/license header and do the imports count as well? However, then lets merge this. I will close the IP-issue for that reason. @ptziegler thanks a lot for this great contribution. |
The current implementation can only show ~5 items at a time. All remaining dependencies have to be selected via a combo box. For projects with several dozen dependencies, it is often faster to edit the XML file directly, rather than using this wizard.
The new implementation keeps all dependencies in a list. Clicking on one of its elements shows more detailed information in a separate page, like GAV, classifier or type.
Further improvements contain the deletion of multiple elements at a time, undo/redo operations, keybindings, as well as a noticeable performance optimizations.
Open Topics
I'd like add some proper tests, but I think this editor is beyond what can be easily tested via JUnit (primarily the interaction between the individual composites). Have there been considerations to add something like RCPTT to the test suite?What's the indented workflow for using images? I'm currently importing them from org.eclipse.ui via ISharedImages. But I've also seen that we have copied some of them directly into the workspace.