8000 PanelListBox Widget by mickes27 · Pull Request #193 · texus/TGUI · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

PanelListBox Widget #193

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.

Sign up for GitHub

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

Jump to bottom
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.

@mickes27
* provided optional index to addItem() method, which allows
fe3e6e0 
   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
texus added a commit 8000 that referenced this pull request Apr 11, 2023
@texus
Minor code cleanup after PR #193
1ab5f44
texus added a commit that referenced this pull request Apr 11, 2023
@texus
Removed PanelListBox::ItemsWidth from PR #193
f31805e
@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 •
edited
Loading

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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mickes27 @texus
0