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

Add insertItem function to ListView widget #138

Merged
merged 4 commits into from
Jul 26, 2020

Conversation

ThalinsGenohan
Copy link
Contributor

Fairly self-explanatory, useful to have; without it, adding an element mid-list would require editing every entry after it.

@texus
Copy link
Owner

texus commented Jul 24, 2020

Thanks for contributing. I do see 2 issues with the code though:

  • You do not check whether the index is within the items. Something like the following should probably be added:
if (index >= m_items.size())
{
    addItem(...);
    return;
}
  • The scrollbar will scroll to the bottom of the list view when m_autoScroll is true, no matter where the items are inserted. Ideally it would scroll to the inserted item, but since nobody has asked for such functionality, you don't have to implement it. I would also accept that you simply don't call setValue at all on the scrollbar, so basically just removing the following code from your functions:
// Scroll down when auto-scrolling is enabled
if (m_autoScroll && (m_verticalScrollbar->getViewportSize() < m_verticalScrollbar->getMaximum()))
    m_verticalScrollbar->setValue(m_verticalScrollbar->getMaximum() - m_verticalScrollbar->getViewportSize());

@ThalinsGenohan
Copy link
Contributor Author

Ah, whoops, you're right. I thought I was missing something. Pushed a fix to both of those, and went ahead and added the autoscroll, since it's just fairly simple, unless I'm missing something else.

@texus
Copy link
Owner

texus commented Jul 25, 2020

The tests are failing to build in c++17 mode. Apparently the vector::emplace function returns an iterator instead of a reference.
So you would need to dereference it so that auto& object = vector.emplace(vector.begin() + index); becomes auto& object = *vector.emplace(vector.begin() + index);

I wasn't going to mention it, but now that I'm asking for another change anyway, maybe you could also change vector.at(index); by vector[index];? The at() function usually isn't used in TGUI (there is no need for out-of-bound exceptions).

Then I will be able to merge the code.

@ThalinsGenohan
Copy link
Contributor Author

I was wondering what the issue there was. Apologies, I've been working a little too much with C# lately, my C++ is a little rusty. Looks like emplace has returned an iterator since C++11, so I'm taking it out of the conditional as well, no need for it to be in there.

@texus texus merged commit d5b980a into texus:0.9-dev Jul 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants