- [FP-2787](https://movai.atlassian.net/browse/FP-2787): IDE - Topics - Can't open multiple Topics tabs #323

quirinpa · 2024-07-12T11:30:06Z

FP-2787: IDE - Topics - Can't open multiple Topics tabs
- Notes:
  - See this comment if you have any doubts
  - Needs: https://github.com/MOV-AI/frontend-npm-ide/pull/594 (with updated lib-ide)

… - Can't open multiple Topics tabs

…t-open-multiple-topics-tabs

…tabs

diasdm

Take a look at the Sonar issues pls

EdwardAngeles · 2024-08-21T09:10:24Z

src/utils/generalFunctions.js

+  const toolIdData = toolIds[toolName] ?? { last: 0, free: [] };
+  const id = toolIdData.free.shift() || toolIdData.last++;
+  const tabData = { ...getToolTabData({ scope: toolName, id: toolName + "-" + id }, props) };
+  if (tabData.multiple) {


Renaming suggestion: multiple -> multipleTabs?
It's easy to know what it means when looking at it from the PR, but by the code itself, it's not that clear.

We've talked about this. I think in the context we could agree it refers to tabs

EdwardAngeles · 2024-08-21T09:11:25Z

src/plugins/views/Tabs/hooks/useTabLayout.js

      return {
        id: id,
+        rid: rid,


What rid stands for? realID? Might be good to rename it...?

realId, yes, but I think it isn't needed.

EdwardAngeles · 2024-08-21T09:12:13Z

src/plugins/views/Tabs/hooks/useTabLayout.js

@@ -356,6 +356,16 @@ const useTabLayout = (props, dockRef) => {

        // Remove tab and apply new layout
        tabsById.current.delete(tabId);
+        const nid = new Number(tabId.substring(tabId.lastIndexOf("-") + 1));


Is nId numberId? subID?

numberId. Renamed

EdwardAngeles · 2024-08-21T09:16:09Z

src/plugins/views/Tabs/hooks/useTabLayout.js

@@ -356,6 +356,16 @@ const useTabLayout = (props, dockRef) => {

        // Remove tab and apply new layout
        tabsById.current.delete(tabId);
+        const nid = new Number(tabId.substring(tabId.lastIndexOf("-") + 1));
+        if (!isNaN(nid)) {
+          const toolIds = tabsById.current.get("toolIds");


Not related to the PR, but We might want to rename tabsById -> tabsByIdRef. Without the ref it looks like we are talking about the "current tab"

In this case I don't think this should be a ref, because a global would suffice. But done.

diasdm

Pls compute the toolId when loading the page

src/tools/index.js

diasdm · 2024-08-21T10:08:33Z

src/utils/generalFunctions.js

+    tabData.name += "-" + id;
+    tabData.title += "-" + id;
+  } else
+    tabData.id = toolName;


Can this be moved to getToolTabData?

The else part can but I don't think it should

quirinpa · 2024-08-21T13:22:15Z

A comment is missing. David asked to compute the toolIds object instead of loading from localStorage

manuelnogueiramov

So I did this review BEFORE you went on vacation, and I kinda forgot to click submit review 🤦
I'm ok with this one as is :)
Good work 👍

manuelnogueiramov · 2024-08-07T15:27:50Z

src/plugins/views/Tabs/hooks/useTabLayout.js

@@ -627,6 +637,7 @@ const useTabLayout = (props, dockRef) => {
      if (!tabFromMemory && !data.content) return;
      const {
        id,
+        rid,


Since you're no longer using rid, we can get rid of it (pun intended!) 😆

manuelnogueiramov · 2024-08-22T11:00:10Z

src/utils/generalFunctions.js

+function breakToolId(tabId) {
+  const index = tabId.search(/\d/);
+  const alpha = tabId.substring(0, index - 1);
+
+  if (index < 0)
+    return [alpha];
+
+  const number = new Number(tabId.substring(index));
+
+  return [alpha, number];
+}
+
+function computeIds(tabsMap) {
+  const res = {};
+
+  for (const [tabId] of tabsMap) {
+    const [alpha, number] = breakToolId(tabId);
+    const index = tabId.search(/\d/);
+    if (index < 0)
+      continue;
+    const specific = res[alpha] ?? { last: 0, busy: {}, free: [] };
+    specific.busy[number] = 1;
+    if (number > specific.last)
+      specific.last = number;
+    res[alpha] = specific;
+  }
+
+  for (const toolName in res) {
+    const specific = res[toolName];
+    for (let j = 0; j < specific.last; j++)
+      if (!specific.busy[j])
+        specific.free.push(j);
+    delete specific.busy;
+    specific.last++;
+  }
+
+  return res;
+}
+
+const workspace = new Workspace();
+const tabsMap = workspace.getTabs();
+const toolIds = computeIds(tabsMap);
+
+export function freeToolId(tabId) {
+  const [alpha, number] = breakToolId(tabId);
+
+  if (!number)
+    return;
+
+  const toolIdData = toolIds[alpha];
+  toolIdData.free.unshift(number);
+}
+


Actually coming to review this again, I'm trying to understand this code, and looks so much more, less performant and more complex than what I did here in the src/utils/Utils.js to achieve the same thing. Can you confirm @EdwardAngeles @quirinpa ?

That only runs when the page loads.
It is to compute the things instead of using localStorage.
After the initial time it runs, it's pretty fast.
Faster than using local storage, for sure.

The fact that goes until the last number doesn't actually add a lot of overhead since we make sure to keep those numbers low. Even if we had 10000 tabs, We wouldn't see a lot of slowness when loading the page.

…t-open-multiple-topics-tabs

manuelnogueiramov

Made a couple of suggestions that may improve the code. Take a look and let me know if you have any questions or would like to do a small pair session.

tl;dr;
Overall, I like the logic of your thinking. I just think you made it harder to understand with generic, non-related naming in most vars and I don't think all this code belongs in this file.

manuelnogueiramov · 2024-08-26T18:51:07Z

src/utils/generalFunctions.js

+function computeIds(tabsMap) {
+  const res = {};
+
+  /* get a map of this format:


Get from where? Maybe you meant create?

We are computing. But also creating.
I don't see this as a strong reason to hold the PR

manuelnogueiramov · 2024-08-26T18:52:06Z

src/utils/generalFunctions.js

+    const index = tabId.search(/\d/);
+    if (index < 0)
+      continue;
+    const specific = res[alpha] ?? { last: 0, busy: {}, free: [] };


What is specific? Maybe since we're doing this only for tool tabs, why not toolTab?

The resulting object is not a tool tab. But indeed something specific to that tool.

manuelnogueiramov · 2024-08-26T18:56:15Z

src/utils/generalFunctions.js

+function breakToolId(tabId) {
+  const index = tabId.search(/\d/);
+  const alpha = tabId.substring(0, index - 1);
+
+  if (index < 0)
+    return [alpha];
+
+  const number = new Number(tabId.substring(index));
+
+  return [alpha, number];
+}


You wouldn't need this entire function if you reused the multiple property to be the tabIncrement like per mine and @diasdm's suggestion. Since you're cycling through the tabs that come from localStorage you'd have access to the entire tabData which includes the multiple, or whatever name we call that property

That seems overly complicated when this simple function suffices perfectly.

manuelnogueiramov · 2024-08-26T18:59:56Z

src/utils/generalFunctions.js

+ * to efficently organize tool ids.
+ */
+function computeIds(tabsMap) {
+  const res = {};


Suggested change

const res = {};

const toolTabIncrementMap = {};

res comes from "result". I don't think "toolTabIncrementMap is a good name.

manuelnogueiramov · 2024-08-26T19:01:08Z

src/utils/generalFunctions.js

+   * }
+   */
+  for (const [tabId] of tabsMap) {
+    const [alpha, number] = breakToolId(tabId);


Suggested change

const [alpha, number] = breakToolId(tabId);

const [toolName, tabIncrement] = breakToolId(tabId);

Also, if you get rid of the breakToolId function, you can use something like:

const {scope, tabIncrement} = tabData;

I don't think there's a strong reason to modify what we're storing there.
A strange idea to be computing part, and using localStorage to store another part.
At least lets be consistent.

manuelnogueiramov · 2024-08-26T19:17:25Z

src/utils/generalFunctions.js

+const workspace = new Workspace();
+const tabsMap = workspace.getTabs();
+const toolIds = computeIds(tabsMap);


I hate that we are just floating around in the middle of this generalFunctions file. I think you might be able to come up with some better way to do this.
Also, maybe all this "tool tab" logic should be in its own file, since none of these are actual "general functions" but very specific ones.

This code is only used here. We already have too many files.

sonarcloud · 2024-08-28T18:25:12Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

diasdm · 2024-09-04T09:01:50Z

Replaced by #334

- [FP-2787](https://movai.atlassian.net/browse/FP-2787): IDE - Topics…

1af29ce

… - Can't open multiple Topics tabs

quirinpa self-assigned this Jul 12, 2024

quirinpa requested a review from a team as a code owner July 12, 2024 11:30

quirinpa force-pushed the FP-2787-ide-topics-cant-open-multiple-topics-tabs branch 3 times, most recently from 5924241 to 1af29ce Compare July 18, 2024 11:15

Paulo Andre Azevedo Quirino and others added 3 commits July 29, 2024 15:02

Merge remote-tracking branch 'origin/dev' into FP-2787-ide-topics-can…

805bdff

…t-open-multiple-topics-tabs

Optional multiple tools windows

4317639

Merge remote-tracking branch 'origin/dev' into FP-2787-ide-topics-can…

9c6053b

…t-open-multiple-topics-tabs

diasdm previously approved these changes Jul 31, 2024

View reviewed changes

diasdm requested a review from manuelnogueiramov July 31, 2024 15:18

diasdm dismissed their stale review via 9c6053b July 31, 2024 15:20

diasdm previously approved these changes Jul 31, 2024

View reviewed changes

manuelnogueiramov mentioned this pull request Aug 1, 2024

FP-2787: Alternative solution #334

Merged

quirinpa dismissed diasdm’s stale review via 12e4776 August 2, 2024 12:50

quirinpa force-pushed the FP-2787-ide-topics-cant-open-multiple-topics-tabs branch from 12e4776 to 72c9aeb Compare August 2, 2024 12:58

Implement reusable ids, use same localStorage item

da801f3

quirinpa force-pushed the FP-2787-ide-topics-cant-open-multiple-topics-tabs branch from 72c9aeb to da801f3 Compare August 2, 2024 13:06

quirinpa added 2 commits August 19, 2024 14:32

Merge branch 'dev' into FP-2787-ide-topics-cant-open-multiple-topics-…

6638b7e

…tabs

Update CHANGELOG.md

6c7f08a

quirinpa requested a review from diasdm August 19, 2024 13:32

diasdm requested changes Aug 20, 2024

View reviewed changes

EdwardAngeles reviewed Aug 21, 2024

View reviewed changes

diasdm requested changes Aug 21, 2024

View reviewed changes

Paulo Andre Azevedo Quirino added 3 commits August 21, 2024 14:04

discussion_r1724717268: remove rid

6b9cc6a

discussion_r1724718360: nid -> numberId

a84e797

discussion_r1724724072: tabsById -> tabsByIdRef

8d88645

Paulo Andre Azevedo Quirino added 2 commits August 21, 2024 14:12

discussion_r1724769292: remove unused var from getToolTabData

ac1ef18

discussion_r1724774129: getTabData -> getNotInstalledTabData

1aa8871

issuecomment-2302048848: Compute toolIds on load

9afe1a9

quirinpa requested review from EdwardAngeles and diasdm August 21, 2024 17:07

manuelnogueiramov previously approved these changes Aug 22, 2024

View reviewed changes

manuelnogueiramov reviewed Aug 22, 2024

View reviewed changes

manuelnogueiramov self-requested a review August 22, 2024 11:00

Add some comments

873e813

quirinpa dismissed manuelnogueiramov’s stale review via 873e813 August 26, 2024 08:11

quirinpa force-pushed the FP-2787-ide-topics-cant-open-multiple-topics-tabs branch from 8b68ced to 873e813 Compare August 26, 2024 08:11

Merge remote-tracking branch 'origin/dev' into FP-2787-ide-topics-can…

997e871

…t-open-multiple-topics-tabs

manuelnogueiramov requested changes Aug 26, 2024

View reviewed changes

quirinpa requested a review from manuelnogueiramov August 27, 2024 08:19

Modified logic a bit to make it more readable

a76b1c6

manuelnogueiramov added a commit that referenced this pull request Sep 3, 2024

Refactored to be more efficient. Added some code suggestions from #323

8aee46f

diasdm closed this Sep 4, 2024

diasdm deleted the FP-2787-ide-topics-cant-open-multiple-topics-tabs branch September 4, 2024 09:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

- [FP-2787](https://movai.atlassian.net/browse/FP-2787): IDE - Topics - Can't open multiple Topics tabs #323

- [FP-2787](https://movai.atlassian.net/browse/FP-2787): IDE - Topics - Can't open multiple Topics tabs #323

quirinpa commented Jul 12, 2024 •

edited

Loading

diasdm left a comment

EdwardAngeles Aug 21, 2024

quirinpa Aug 21, 2024

EdwardAngeles Aug 21, 2024

quirinpa Aug 21, 2024

EdwardAngeles Aug 21, 2024

quirinpa Aug 21, 2024 •

edited

Loading

EdwardAngeles Aug 21, 2024

quirinpa Aug 21, 2024

diasdm left a comment

diasdm Aug 21, 2024

quirinpa Aug 21, 2024 •

edited

Loading

quirinpa commented Aug 21, 2024

manuelnogueiramov left a comment

manuelnogueiramov Aug 7, 2024

manuelnogueiramov Aug 22, 2024

quirinpa Aug 22, 2024 •

edited

Loading

manuelnogueiramov left a comment

manuelnogueiramov Aug 26, 2024

quirinpa Aug 27, 2024

manuelnogueiramov Aug 26, 2024

quirinpa Aug 27, 2024

manuelnogueiramov Aug 26, 2024

quirinpa Aug 27, 2024

manuelnogueiramov Aug 26, 2024

quirinpa Aug 27, 2024

manuelnogueiramov Aug 26, 2024

manuelnogueiramov Aug 26, 2024

quirinpa Aug 27, 2024 •

edited

Loading

manuelnogueiramov Aug 26, 2024

quirinpa Aug 27, 2024

sonarcloud bot commented Aug 28, 2024

diasdm commented Sep 4, 2024

	const [alpha, number] = breakToolId(tabId);
	const [toolName, tabIncrement] = breakToolId(tabId);

- [FP-2787](https://movai.atlassian.net/browse/FP-2787): IDE - Topics - Can't open multiple Topics tabs #323

- [FP-2787](https://movai.atlassian.net/browse/FP-2787): IDE - Topics - Can't open multiple Topics tabs #323

Conversation

quirinpa commented Jul 12, 2024 • edited Loading

diasdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quirinpa Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diasdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quirinpa Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

quirinpa commented Aug 21, 2024

manuelnogueiramov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quirinpa Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

manuelnogueiramov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quirinpa Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 28, 2024

Quality Gate passed

diasdm commented Sep 4, 2024

quirinpa commented Jul 12, 2024 •

edited

Loading

quirinpa Aug 21, 2024 •

edited

Loading

quirinpa Aug 21, 2024 •

edited

Loading

quirinpa Aug 22, 2024 •

edited

Loading

quirinpa Aug 27, 2024 •

edited

Loading