PR: qta-browser update #233

pedromorgan · 2023-05-04T22:50:27Z

Some qta-browser updates to make gui nicer.

A toolbar at top with actions
make stuff persist with qsettings

Hidden Motive

End goal is to "embed as widget in app" and let clients decide their own icons and colors ;-)

commit aabecef Author: pedromorgan@gmail.com <--replace-all> Date: Thu May 4 22:42:59 2023 +0100 GUI improvements for necessitarty commit 779fde3 Author: pedromorgan@gmail.com <--replace-all> Date: Thu May 4 22:27:08 2023 +0100 clear button commit f47909f Author: pedromorgan@gmail.com <--replace-all> Date: Thu May 4 22:10:25 2023 +0100 toolbar group commit 6314863 Author: pedromorgan@gmail.com <--replace-all> Date: Thu May 4 21:38:40 2023 +0100 toolbar commit 6009b48 Author: pedromorgan@gmail.com <--replace-all> Date: Thu May 4 21:16:24 2023 +0100 cols and settings commit 22d6cc3 Author: pedromorgan@gmail.com <--replace-all> Date: Thu May 4 20:42:23 2023 +0100 add cols size coil

dalthviz · 2023-05-16T22:28:30Z

Hi @pedromorgan thank you for your interest in improving the icon browser! I think the idea behind the changes makes sense but just in case, what do you think @ccordoba12 ?

Also, if this go forward I think we should update the screenshot for the icon browser and maybe expand a little bit the README section about it (maybe including info about the settings that are persisted between launches?) and of course fix the failing tests :)

Thank you again for your interest in improving QtAwesome @pedromorgan !

pedromorgan · 2023-05-19T03:36:06Z

hi @dalthviz, thanks for reply... and an updates patch

fixed the tests
removed settings, cos its not really necessary, as before

ccordoba12 · 2023-06-07T22:19:49Z

I looks good to me too, thanks @pedromorgan for your contribution!

pedromorgan · 2023-06-08T22:25:26Z

Just updated screenshot

Ready for merge.. TIA

pedromorgan · 2023-06-08T22:36:36Z

@dalthviz @ccordoba12 How does this review work?

dalthviz · 2023-06-13T00:35:21Z

Hi @pedromorgan sorry for the late response! Just approved the run of the CI to check that the tests are passing (and seems like everything is green now 🎉 ) and now we will give this a more thoughtful review 👍

Just in case, after a quick check, looks like maybe the only couple of things I would say needs to be changed are:

The icon of the window - Although pandas are nice, maybe using the fa5s.icons or fa5b.font-awesome icon could be better:

Maybe the number of default columns (maybe to 6 or 8), since from the screenshot 10 seems like to much.

Anyhow, I would check this locally too as part of the review and maybe then the things above could not be so interesting to do or maybe some other suggestions could appear.

pedromorgan · 2023-06-13T02:05:29Z

@dalthviz fixed the icon...

Re columns, am using 30 across on a big screen.. the main reason I hacked in the first place..

dalthviz · 2023-06-21T00:56:41Z

Re columns, am using 30 across on a big screen.. the main reason I hacked in the first place..

Checked locally and actually I think the default columns number is okay 👍 However, the window minimum size is kind of big (at least for my screen) so probably we will need to reduce it a little bit. Locally I'm seeing:

I will leave some suggestions as a review to change the minimum size and some other little nitpicks (like the clear icon and maybe using tooltips instead of the addition of the ToolBarGroup class/instances as titles)

pedromorgan · 2023-06-21T01:01:46Z

@dalthviz Done.. Fixed to minimum screen size ;-)))))))))))))

dalthviz

Thank you again @pedromorgan for the work here! Left some initial comments regarding the widgets properties, move the list with possible number of columns to be a constant and also a way to simplify the code to clear the filter line edit.

Probably the biggest change that I propose in this review is to drop the usage of the custom class ToolBarGroup and just set tooltips for the different widgets, but let me know what do you think!

dalthviz · 2023-06-21T01:36:28Z

qtawesome/icon_browser.py

+        buttClear = QtWidgets.QToolButton()
+        buttClear.setAutoRaise(True)
+        buttClear.setToolButtonStyle(QtCore.Qt.ToolButtonIconOnly)
+        buttClear.setIcon(qtawesome.icon("mdi.alpha-x"))
+        buttClear.clicked.connect(self._clearClicked)
+        tbgFont.addWidget(buttClear)


Thinking about this, I think the clear button logic can be removed and instead we could use here the QLineEdit setClearButtonEnabled method: https://doc.qt.io/qt-6/qlineedit.html#clearButtonEnabled-prop

So adding a line for the _lineEditFilter like:

self._lineEditFilter.setClearButtonEnabled(True)

Instead of the extra widget for the button. With this change you should be able to see something like:

dalthviz · 2023-06-21T01:42:19Z

qtawesome/icon_browser.py

+        tbcols = ToolBarGroup("Columns")
+        toolbar.addWidget(tbcols)
+        self._combo_cols = QtWidgets.QComboBox(self)
+        for idx, no in enumerate([5, 8, 10, 15, 20, 25, 30]):


Lets move move this list with the possible values for the number of columns to a constant. Something like NUM_COLUMNS_OPTIONS = [5, 8, 10, 15, 20, 25, 30] or maybe even NUM_COLUMNS_OPTIONS = [5, 10, 15, 20, 25, 30] since now the the default is 5

dalthviz · 2023-06-21T20:57:43Z

qtawesome/icon_browser.py


        self._nameField = QtWidgets.QLineEdit(self)
        self._nameField.setAlignment(QtCore.Qt.AlignCenter)
        self._nameField.setReadOnly(True)
+        self._nameField.setFixedWidth(250)


Lets change this fixed width to be a maximum width so when resizing the window to the minimum size widgets don't overlap:

Suggested change

self._nameField.setFixedWidth(250)

self._nameField.setMaximumWidth(250)

dalthviz · 2023-06-21T20:58:56Z

qtawesome/icon_browser.py

+        tbgFont.addWidget(self._comboFont)
+
+        self._lineEditFilter = QtWidgets.QLineEdit(self)
+        self._lineEditFilter.setFixedWidth(200)


Lets change this fixed width to be a maximum width so when resizing the window to the minimum size widgets don't overlap:

Suggested change

self._lineEditFilter.setFixedWidth(200)

self._lineEditFilter.setMaximumWidth(200)

dalthviz · 2023-06-21T21:51:35Z

qtawesome/icon_browser.py

+        tbgFont = ToolBarGroup("Filter")
+        toolbar.addWidget(tbgFont)
+        self._comboFont = QtWidgets.QComboBox(self)
+        self._comboFont.setFixedWidth(75)


Lets change this fixed width to be a maximum width so when resizing the window to the minimum size widgets don't overlap:

Suggested change

self._comboFont.setFixedWidth(75)

self._comboFont.setMaximumWidth(75)

dalthviz · 2023-06-21T21:56:21Z

qtawesome/icon_browser.py

+
+        # selected group
+        tbgSelected = ToolBarGroup("Selected")
+        toolbar.addWidget(tbgSelected)


Lets add some stretch here to enable the line edit to expand until reaching its maximum width when needed:

Suggested change

toolbar.addWidget(tbgSelected)

toolbar.addWidget(tbgSelected, stretch=10)

dalthviz · 2023-06-21T21:57:05Z

qtawesome/icon_browser.py

+
+        # filter group
+        tbgFont = ToolBarGroup("Filter")
+        toolbar.addWidget(tbgFont)


Lets add some stretch here to enable the line edit to expand until reaching its maximum width when needed:

Suggested change

toolbar.addWidget(tbgFont)

toolbar.addWidget(tbgFont, stretch=10)

dalthviz · 2023-06-21T22:08:54Z

qtawesome/icon_browser.py

@@ -260,6 +311,31 @@ def data(self, index, role):
        return super().data(index, role)


+class ToolBarGroup(QtWidgets.QWidget):


Regarding this custom widget to add titles, what do you think if instead we just add tooltips to the relevant widgets so people can check/understand what they do?

So for example for the line edit that filters the icons:

For the text for each element, it could be something like:

Font combobox: Select fonts prefix which icons will be included in the filtering

Filter line edit: Filter icons by name

Selected icon line edit: Full identifier of the currently selected icon

Copy button: Copy selected icon full identifier to the clipboard

Theme combobox: Select color palette for the icons and the icon browser

Columns combobox: Select number of columns the icons list is showing

dalthviz · 2023-06-21T22:10:54Z

qtawesome/tests/test_icon_browser.py

    """
-    Ensure the filter UX works
+    What happens if a `penguin`


Lets rewrite this to:

Suggested change

What happens if a `penguin`

Ensure the filter UX works when searching for `penguin`

dalthviz · 2023-06-21T22:12:43Z

qtawesome/tests/test_icon_browser.py


+def test_filter_fail(qtbot, browser):
+    """
+    What happens if a `not a penguin-penguin`


Lets rewrite this to be:

Suggested change

What happens if a `not a penguin-penguin`

Ensure the filter doesn't show results if the filter text doesn't match any icon

ccordoba12 · 2023-06-22T01:34:24Z

@pedromorgan, may I ask: why did you decide to close this pull request? We can help you finish it, if that's ok for you.

pedromorgan · 2023-06-22T16:56:04Z

Because I got better things to do than fix some review on code style etc. Also this is the first time in the discussion that a lot of the topics are brought up.

The time spent doing the review, could have been spent fixing the code itself.

So am not going to waste my time.. TA

pedromorgan · 2023-06-22T22:27:23Z

I understand your role now.. A pointless job..

So instead of actually doing things,
instead telling people to do things..
instead of doing it themselves...

AFAIK..

u should have merges it in your own branch
clean it up to "YOUR" taste
and then commit to main and job done..
fingers crossed and job done

This explains a lot why foss project stall..

and ultimately fork
over a trivial issue

pedromorgan · 2023-06-22T22:29:33Z

@dalthviz has spent more time revieing my code, that actually correct the errors within...

There is a weed in the garden

I propose to remove it...
No wait. .need to check first with code review

dalthviz · 2023-06-22T23:45:16Z

I'm sorry @pedromorgan if the way I reviewed the PR frustated you. Just until yestarday I was able to make time to give a full review and it ended up having more changes/suggestions than I initially though. I still think that the work here is worthy to be finished and merged so if you are okay I can help finish the PR so it can be merged.

Again, truly sorry if the review process was not what you expected to be. By no means I wanted you to feel bad or frustated with this PR.

pedromorgan · 2023-07-05T22:46:34Z

@derek-rein Why not just merge this, and go and fix it.. TIA.. ;-))))

U spent more time on review, instead of taking the "kinda way i see it".. to fit your review ideals
instead of merging it,, and then fixing it... even though my version is better than original > its now ever better..

Move faster please... and let ideas flow... Think more like a carpenter and wood rather than code..

pedromorgan mentioned this pull request May 4, 2023

qta-browser "Empty key passed" #231

Closed

dalthviz changed the title ~~qta-browser update~~ PR: qta-browser update May 16, 2023

dalthviz added type:Enhancement type:Discussion labels May 16, 2023

fix tests and remote settings

3e2d217

pedromorgan@gmail.com added 3 commits June 7, 2023 23:37

penguin

cd1a387

Update qta-brwoser

eaba2d3

updated qta-browser

4c463d3

dalthviz assigned pedromorgan Jun 13, 2023

dalthviz self-requested a review June 13, 2023 00:35

dalthviz added this to the v1.3.0 milestone Jun 13, 2023

qta-browser updates

2cdcd52

qta-browser screenshot

42cab9f

qta-browser update

0f41ce5

dalthviz reviewed Jun 21, 2023

View reviewed changes

pedromorgan closed this Jun 21, 2023

dalthviz removed this from the v1.3.0 milestone Jun 21, 2023

pedromorgan reopened this Jun 22, 2023

dalthviz mentioned this pull request Nov 23, 2023

PR: Improvements to the icon browser application #248

Merged

ccordoba12 merged commit 0f41ce5 into spyder-ide:master Nov 24, 2023
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: qta-browser update #233

PR: qta-browser update #233

pedromorgan commented May 4, 2023

dalthviz commented May 16, 2023

pedromorgan commented May 19, 2023

ccordoba12 commented Jun 7, 2023

pedromorgan commented Jun 8, 2023

pedromorgan commented Jun 8, 2023

dalthviz commented Jun 13, 2023 •

edited

Loading

pedromorgan commented Jun 13, 2023

dalthviz commented Jun 21, 2023

pedromorgan commented Jun 21, 2023

dalthviz left a comment

dalthviz Jun 21, 2023

dalthviz Jun 21, 2023

dalthviz Jun 21, 2023

dalthviz Jun 21, 2023

dalthviz Jun 21, 2023

dalthviz Jun 21, 2023

dalthviz Jun 21, 2023

dalthviz Jun 21, 2023

dalthviz Jun 21, 2023

dalthviz Jun 21, 2023 •

edited

Loading

ccordoba12 commented Jun 22, 2023

pedromorgan commented Jun 22, 2023

pedromorgan commented Jun 22, 2023

pedromorgan commented Jun 22, 2023

dalthviz commented Jun 22, 2023

pedromorgan commented Jul 5, 2023

	self._nameField.setFixedWidth(250)
	self._nameField.setMaximumWidth(250)

	self._lineEditFilter.setFixedWidth(200)
	self._lineEditFilter.setMaximumWidth(200)

	self._comboFont.setFixedWidth(75)
	self._comboFont.setMaximumWidth(75)

	toolbar.addWidget(tbgSelected)
	toolbar.addWidget(tbgSelected, stretch=10)

	toolbar.addWidget(tbgFont)
	toolbar.addWidget(tbgFont, stretch=10)

		@@ -260,6 +311,31 @@ def data(self, index, role):
		return super().data(index, role)


		class ToolBarGroup(QtWidgets.QWidget):

	What happens if a `penguin`
	Ensure the filter UX works when searching for `penguin`

	What happens if a `not a penguin-penguin`
	Ensure the filter doesn't show results if the filter text doesn't match any icon

PR: qta-browser update #233

PR: qta-browser update #233

Conversation

pedromorgan commented May 4, 2023

dalthviz commented May 16, 2023

pedromorgan commented May 19, 2023

ccordoba12 commented Jun 7, 2023

pedromorgan commented Jun 8, 2023

pedromorgan commented Jun 8, 2023

dalthviz commented Jun 13, 2023 • edited Loading

pedromorgan commented Jun 13, 2023

dalthviz commented Jun 21, 2023

pedromorgan commented Jun 21, 2023

dalthviz 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

dalthviz Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

ccordoba12 commented Jun 22, 2023

pedromorgan commented Jun 22, 2023

pedromorgan commented Jun 22, 2023

pedromorgan commented Jun 22, 2023

dalthviz commented Jun 22, 2023

pedromorgan commented Jul 5, 2023

dalthviz commented Jun 13, 2023 •

edited

Loading

dalthviz Jun 21, 2023 •

edited

Loading