Introduce TerminalViewId instead of nullable id and secondaryId#2030
Introduce TerminalViewId instead of nullable id and secondaryId#2030laeubi merged 1 commit intoeclipse-platform:masterfrom
TerminalViewId instead of nullable id and secondaryId#2030Conversation
ruspl-afed
force-pushed
the
terminal_view_id
branch
from
14239ab to
7c37220
Compare
|
@laeubi this is what I would like to suggest as further step for our discussion |
| public TerminalViewId next() { | ||
| return new TerminalViewId(primary, nextSecondaryId()); | ||
| } |
There was a problem hiding this comment.
All call-sites of next() seem to perform new TerminalId().next() so it this really suitable here as an instance method and should not be more a static method like
| public TerminalViewId next() { | |
| return new TerminalViewId(primary, nextSecondaryId()); | |
| } | |
| public static TerminalViewId create() { | |
| return new TerminalViewId(IUIConstants.ID, nextSecondaryId(IUConstants.ID)); | |
| } |
There was a problem hiding this comment.
I'm not so sure regarding this.
We allow to specify primary id for some reason, perhaps we expect terminal view with another view id to use this API (may be similar with what we have for "Problem" view family).
...al/bundles/org.eclipse.terminal.view.ui/src/org/eclipse/terminal/view/ui/TerminalViewId.java
Outdated
Show resolved
Hide resolved
...al/bundles/org.eclipse.terminal.view.ui/src/org/eclipse/terminal/view/ui/TerminalViewId.java
Outdated
Show resolved
Hide resolved
| * @see IUIConstants#ID | ||
| * @see IViewReference#getSecondaryId() | ||
| */ | ||
| public final class TerminalViewId { |
There was a problem hiding this comment.
Should we probably make this a record class?
There was a problem hiding this comment.
I thought we don't want to use record for API types. But ok, I did.
There was a problem hiding this comment.
@HannesWell @akurtakov do we have any such agreement for platform?
ruspl-afed
force-pushed
the
terminal_view_id
branch
from
7c37220 to
b03b3f8
Compare
|
Thank you for your review @laeubi |
|
Sadly infrastructure is still down :- @jonahgraham any thoughts about having different primary view IDs? |
|
@ruspl-afed there are three javadoc issues to be fixed: https://ci.eclipse.org/platform/job/eclipse.platform/job/PR-2030/4/javadoc-warnings/new/ |
ruspl-afed
force-pushed
the
terminal_view_id
branch
from
b03b3f8 to
796867d
Compare
Currently there is a pair of arguments in a number of signatures `String id, String secondaryId` each of which may be `null`. Instead of this pair a new type introduced that also handles the logic of calculation for the next secondary id. As a result the client code much be simplified to look like this: `UIPlugin.getConsoleManager().showConsoleView(new TerminalViewId().next());`
ruspl-afed
force-pushed
the
terminal_view_id
branch
from
796867d to
bebe6a5
Compare
@laeubi everything is green now, I suggest to merge this PR and then create others if needed |
2 participants
Currently there is a pair of arguments in a number of signatures
String id, String secondaryIdeach of which may benull.Instead of this pair a new type introduced that also handles the logic of calculation for the next secondary id.
As a result the client code may be simplified to look like this:
UIPlugin.getConsoleManager().showConsoleView(new TerminalViewId().next());