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

PanelListBox Widget #193

Merged
merged 6 commits into from Apr 11, 2023
Merged

PanelListBox Widget #193

merged 6 commits into from Apr 11, 2023

Conversation

mickes27
Copy link
Contributor

@mickes27 mickes27 commented Apr 2, 2023

This is a pull request with PanelListBox Widget. The idea for this widget is to provide more flexibility for ListBox, so user can use Panel instead of Text.

Example code:

const auto panelListBox = tgui::PanelListBox::create();
panelListBox->setPosition(10, 10);
panelListBox->setSize(300, 400);

const auto panel1 = panelListBox->addItem();
const auto panel2 = panelListBox->addItem();
const auto panel3 = panelListBox->addItem();
panelListBox->addItem();
panelListBox->addItem();
panelListBox->addItem();

const auto button = tgui::Button::create("Delete");
panel1->add(button);

const auto pic = tgui::Picture::create("Texture1.png");
panel2->add(pic);

const auto label = tgui::Label::create("Some panel");
panel3->add(label);

gui.add(panelListBox);

gui.mainLoop();

Which results in following PanelListBox:

obraz

There are still some functions which are missing, for example:

  • Chaning items in place
  • Using arrow keys to navigate list
  • Double clicks
  • Mouse press/release events

I'd be glad for any comments how can I improve this code.

@texus
Copy link
Owner

texus commented Apr 2, 2023

Thank you for contributing this. I didn't have time yet to look at it in-depth and to test the code, but on first sight it already looks great except for some minor details.

  • The code style for if/else statements and loops doesn't match the rest of TGUI. One line in a body doesn't require braces, but if there are braces then they should always be on a separate line (see https://www.sfml-dev.org/style.php#parenthesis-braces)
  • I would rename SignalPanel to either SignalSelectedPanel or SignalPanelListBoxItem, to make it a bit less vague.
  • Changing items in place is probably not needed, but a function should exist to insert a panel at given an index.
  • Double clicks and mouse press/release events: those are nice to have features, but as long as nobody needs them then I don't consider them as required functionality.
  • One other nice to have would be that you could change the template, so that you could add certain widgets once and they would appear on all panels created afterwards.

   to insert new element at certain position
 * getPanelTemplate() which allows for customization newly created
   elements
 * rename SignalPanel to SignalPanelListBoxItem
 * fix formatting issues
@mickes27
Copy link
Contributor Author

mickes27 commented Apr 4, 2023

Added new commit. Fixed the formatting errors (now it should follow SFML guidelines). Also, added method for getting panel template, so it can be freely changed. I was thinking about using some kind of void changePanelTemplate(Panel::Ptr template), however I think that just returning used template gives us more flexibility (we already have eveyrthing we were using, so there is no need to additional getting existing widgets and customization)

@texus texus merged commit 8f68af6 into texus:0.10 Apr 11, 2023
11 checks passed
texus added a commit that referenced this pull request Apr 11, 2023
texus added a commit that referenced this pull request Apr 11, 2023
@texus
Copy link
Owner

texus commented Apr 11, 2023

Thanks again for creating the widget. I did make a few minor changes when I merged it:

  • While testing I found one minor issue: if setSize wasn't called then the items didn't work properly. I've fixed this by having a setSize call at the end of the constructor.
  • There were quite some lines with only whitespace on them.
  • I removed the ItemsWidth property. I missed its existence during the review and my fix with setSize in the constructor caused tests to fail, so I had to either quickly fix ItemsWidth or remove it. I don't really believe it should exist, and it's implementation wouldn't have worked when the layout parameter wasn't a constant, but was there perhaps a use case that you had in mind for which you added it?

@mickes27
Copy link
Contributor Author

mickes27 commented Apr 11, 2023

I introduced ItemsWidth property for more flexibility (it was in first iteration, where horizontal scrollbar was still used). In that case user could potentially use larger panels than list box size. I think that it was unnecessary, if we decide that horizontal scrollbar is never shown and we should not exceed whole widget size.

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.

None yet

2 participants