-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
107-server-buttons #108
107-server-buttons #108
Conversation
201572f
to
133e1fb
Compare
|
133e1fb
to
261a5f6
Compare
83fd603
to
a1db1cd
Compare
a1db1cd
to
49b903f
Compare
@@ -85,7 +84,61 @@ | |||
|
|||
private static boolean loading = false; | |||
|
|||
private static class NavigationHistory<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the concept... Will explain it later. However let me put comments to this specific implementation (as I have comments to it as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StudioPanel is already overloaded class. It doesn't make sense for me to add inner class. It should be separate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and btw, we need tests. Non UI class should be very simple to test
public void addSelectedItem(T item) { | ||
if (item.equals(getSelectedItem())) | ||
return; | ||
while (history.size() > index + 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't you want to code while (hasNextItem() ) ?
public void removeSelectedItem() { | ||
if (!history.isEmpty()) { | ||
history.remove(index); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- what will happen if the index was the last element?
- what selectedItem is pointed to now?
} | ||
|
||
public T getSelectedItem() { | ||
return selectedItem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like idea of caching selectedItem. There is no any speed benefit, but you always need to make sure that index and selectedItem correspond to each other. I would do not have selectedItem field; but having here history.get(index) instead
return index != 0; | ||
} | ||
|
||
public void setPreviousItem() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't like the naming. You are not setting the next item. IMHO, the better name is "previous()" or "moveBack()"
if (addToNavigationHistory) | ||
serverNavigateHistory.addSelectedItem(server); | ||
prevServerAction.setEnabled(serverNavigateHistory.hasPreviousItem()); | ||
nextServerAction.setEnabled(serverNavigateHistory.hasNextItem()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And below (hidden in the review) is the reason I don't like the concept of NavigationHistory (which I already advertised earlier). We simply already have serverHistory. I think we need one navigation or serverHistory. The logic should be combined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and serverHistoryAction should move a selectedIndex pointer in the navigationHistory, rather than adding an item in the end of the navigation
|
||
if (addToNavigationHistory) | ||
serverNavigateHistory.addSelectedItem(server); | ||
prevServerAction.setEnabled(serverNavigateHistory.hasPreviousItem()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to put logic for actions enable/disable into a single place - refreshActionState()
comboServer.setSelectedItem(serverNavigateHistory.getSelectedItem()); | ||
setServer(serverNavigateHistory.getSelectedItem(), false); | ||
}); | ||
prevServerAction.setEnabled(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to put all action.setEnabled into refreshActionState()
@@ -788,6 +849,20 @@ private void initActions() { | |||
serverListAction = UserAction.create(I18n.getString("ServerList"), Util.TEXT_TREE_ICON, "Show server list", | |||
KeyEvent.VK_L, KeyStroke.getKeyStroke(KeyEvent.VK_L, menuShortcutKeyMask | InputEvent.SHIFT_MASK), | |||
e -> showServerList(false)); | |||
prevServerAction = UserAction.create("Previous server", Util.BACK_ICON, "Previous server", 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to have some shortcuts (didn't come up what exact shortcut)
@@ -741,8 +796,14 @@ else if ((row == 1) && ((noWins % 3) > 1)) | |||
} | |||
} | |||
|
|||
private void setServer(Server server) { | |||
if (server == null) return; | |||
private void setServer(Server server, boolean addToNavigationHistory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is what I spot but think a lot how I would do it here..
I don't like the boolean addToNavigationHistory argument. As we have different use-cases - replace the current item, add to the tail, move back and forward.
May be we need to have setServer(server) meaning we have a new Server which we want to set and add to the tail; and a separate method navigate(boolean moveForward) which is used for navigation?
But overall - the functionality is definitely useful. And it was requested by users. |
Fixes #107
The icons are taken from KDE project (Breeze theme)