Skip to content
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

Replace Q3PtrList in diagram dialog #854

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

wawuwo
Copy link
Contributor

@wawuwo wawuwo commented Jul 22, 2024

Hi!

This is another piece for #748

@wawuwo wawuwo force-pushed the replace-q3ptrlist-in-diagram-dialog branch from 1b6f4da to b3deafb Compare July 22, 2024 05:49
@ra3xdh ra3xdh added this to the 24.4.0 milestone Jul 22, 2024
@dsm
Copy link
Collaborator

dsm commented Jul 22, 2024

Hi, can we use QList instead of std::vector for this refactoring.

@wawuwo
Copy link
Contributor Author

wawuwo commented Jul 22, 2024

@dsm hi! Is there a specific reason for it? QList and std::vector both array-based, Graph is a private member of diagram dialog and is not passed as argument to any of QT APIs

@dsm
Copy link
Collaborator

dsm commented Jul 22, 2024

Just looking your modification and the line in diagramdialog.cpp Graphs.erase(std::next(Graphs.begin(), i));, I'm questioning QList has a remove method ? maybe QList usage just simplify something like that in codes. It's just advice. but std::vector is ok :)

@ra3xdh ra3xdh linked an issue Jul 22, 2024 that may be closed by this pull request
4 tasks
@wawuwo
Copy link
Contributor Author

wawuwo commented Jul 22, 2024

Now I see what you mean

I agree std::vector's iterator-based API is a bit clumsy… and QList indeed does have a nice remove method 🤔

@dsm
Copy link
Collaborator

dsm commented Jul 22, 2024

Btw I Quick test QList over std::vector some compiler error happend, I think QList wants to copy uniquie ptr somewhere I don't know where but unique ptr doesn't copyable just movable maybe we can use weak_ptr or shared_ptr instead of unique if there isn't spesific desicion.

@wawuwo
Copy link
Contributor Author

wawuwo commented Jul 22, 2024

I am more inclined to keep the std::vector for this one… It works nice and easy in all cases except of removing an element, and here is only one case of removing (plus I just like to stick with standard library facilities when possible). But anyway, thanks for the advice and attention to details

@ra3xdh ra3xdh removed a link to an issue Jul 26, 2024
4 tasks
@ra3xdh ra3xdh merged commit 3f2fee3 into ra3xdh:current Jul 26, 2024
7 checks passed
@ra3xdh
Copy link
Owner

ra3xdh commented Jul 26, 2024

Merged. I didn't find issues while testing this.

@ra3xdh ra3xdh linked an issue Jul 26, 2024 that may be closed by this pull request
4 tasks
@wawuwo wawuwo deleted the replace-q3ptrlist-in-diagram-dialog branch July 26, 2024 15:06
@ra3xdh ra3xdh mentioned this pull request Aug 2, 2024
4 tasks
@ra3xdh ra3xdh modified the milestones: 24.4.0, 24.3.1 Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of Q3PtrList wrapper
3 participants