Use tab id and order field where appropriate instead of tabIndex. #1462
Conversation
shubham-padia
force-pushed
the
tab-index
branch
9 times, most recently
from
c8b8e6e to
f194e7c
Compare
For `data-tab-id`, we were using tabIndex as a unique numeric incremental identifier. Having the word index in it was confusing since we have other index fields which do act like indexes.
This id will be passed around in events to uniquely identify a server. This id is not user generated, it is generated in app by domain-util.ts and stored in our JSON DB. This commit just adds the logic for generating and storing the id in the JSON DB. This commit does not make use of this newly added id, that will be done in further commits to keep the commits readable. In further commits, we will also start setting tabId in the rendered code to this id. We generate a random hex string for this id instead of having a numeric incremental ID. The reason for this is to avoid overhead of maintaining an ever increasingly numeric id while trying to make sure that ID collisions don't happen. Since we don't store our data in a real database, we can't have incremental primary keys by default and it makes little sense to implement that on our own, when this field is just being passed around to communicate b/w code and is not shown to humans.
We want to start using server id to communicate the identity of server in events. This commit does that for `switch-server-tab`. It also requires us to set tabId to server id instead of an incremental number. We try to make changes in this commit minimal and pass index to the functions that switch-server-tab calls. We will replace that index with tabId in further commits incrementally.
Use id instead of index to get and update domain. We remove updateDomain in this commit but not getDomain since it is being used in other places, where it will be easier to remove when changing the encompassing functions to use id instead of index.
shubham-padia
force-pushed
the
tab-index
branch
2 times, most recently
from
e153ec7 to
652bc0a
Compare
shubham-padia
changed the title
We also use lastActiveTabId instead of lastActiveTabIndex now. `save-last-tab` uses id instead of index to commmunicate the identity of a server. getNextServer and getPreviousServer also start using id with this commit, the index mentioned in those functions is actually the order in which the servers should be displayed and we will fix that problem in upcoming commits.
We also replace pending getDomain() removals in favour of getDomainById() in this commit. We add a new function called removeDomainById as well.
This commit removes miscallenous instances of accessing tabs directly by the index.
After introducing id field to the server object, index was just an indicator of the ordering of tabs in the tab list. This commit changes the name to accurately represent the field's function. This commit should make it easier to change the order of tabs in the future. Since order is not the same as index anymore, we add an additional function called getTabByOrder to replace the leftover instances of us fetching a tab by it's index when it meant to fetch a tab by it's id. After this commit, we should not have any instances of accessing tabs directly by their index in the server list or the tab list.
shubham-padia
force-pushed
the
tab-index
branch
from
652bc0a to
b210bbd
Compare
| "show-keyboard-shortcuts": () => void; | ||
| "show-notification-settings": () => void; | ||
| "switch-server-tab": (index: number) => void; | ||
| "switch-server-tab": (id: string) => void; |
There was a problem hiding this comment.
Is this a server ID or a tab ID? (Are those two different things?)
It'd be good to make the answer explicit: use specific names like serverId or tabId, rather than just id, everywhere except the ID property that's right on the object it identifies. (And then if it seems most convenient to use the same name there too, that's also a popular choice.)
There was a problem hiding this comment.
Is this a server ID or a tab ID? (Are those two different things?)
We set tab ID to server ID in the app for server tabs, but not the other way around. For functional tabs, tabId is just a unique string ID.
tabId would be more appropriate to use here, since we are talking about switching tabs.
| export type ServerConfig = { | ||
| id: string; |
There was a problem hiding this comment.
OTOH if the ID property here is just the tab ID of some tab, rather than an independent ID for the server, then this should get a name like tabId that makes that explicit.
There was a problem hiding this comment.
It is an independent ID for the server. We set tabId to that independent id in the app.
Now that I think about it, if this seems confusing, it might be good to have two fields on the tab object, tabId can be a unique ID that is independent of the server id. And we store server id in a separate field called serverId, which will be undefined in case of functional tabs. Functional tabs are settings, reload, etc.
Let me know what you think of the current approach vs the proposed one vs anything else you might have in mind based on the explanation above, thanks!
There was a problem hiding this comment.
Good question. Probably a good discussion to have in a chat thread — that'll make it more visible for Anders and/or Tim to chime in too.
There was a problem hiding this comment.
3 participants
Screenshots and screen captures:
Platforms this PR was tested on:
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: