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

Implement tag renaming #1165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

pinkavaj
Copy link
Contributor

@pinkavaj pinkavaj commented Nov 8, 2022

No description provided.

@vladisslav2011
Copy link
Contributor

Isn't it better to just actually rename the tag (update TagInfo::name, this should not invalidate pointers) instead of looping through all bookmarks updating the pointer to a tag?
I think, the CI wants #include <QDebug>.

@vladisslav2011
Copy link
Contributor

I've fixed the CI https://github.com/vladisslav2011/gqrx/tree/pi-rename-tag, but it does not work as expected. It tries to create a new tag instead of renaming and does wrong things when a tag is renamed to an existing name. I think, renaming should fail if the name, we are trying to rename to, already exists...

@vladisslav2011
Copy link
Contributor

I've added another commit to https://github.com/vladisslav2011/gqrx/tree/pi-rename-tag
It works like it should now.
I think, we should remove 'Create New Tag' menu item from tag filter window as it does not work as expected (it does not actually create a tag, but adds it to the list and allows to filter using it).

@pinkavaj
Copy link
Contributor Author

pinkavaj commented Nov 9, 2022

I've added another commit to https://github.com/vladisslav2011/gqrx/tree/pi-rename-tag It works like it should now. I think, we should remove 'Create New Tag' menu item from tag filter window as it does not work as expected (it does not actually create a tag, but adds it to the list and allows to filter using it).

If there is an option to delete tag as user I would expect it will be also possible to create a tag. Yes, it is not working right now, but that can be fixed.

@vladisslav2011
Copy link
Contributor

Adding a tag and not assigning it to a bookmark looks pointless to me. It is still possible to create a 'floating' tag, that is not assigned to a bookmark (add a tag to a single bookmark, then remove it from the bookmark), but this tag will always get filtered out in Bookmarks::save().

@pinkavaj
Copy link
Contributor Author

pinkavaj commented Nov 9, 2022

Lets keep the focus on the subject of renaming.

Thx @vladisslav2011 for the suggestions, I have kind of backported them to the code and tested and they work like a charm.

@pinkavaj
Copy link
Contributor Author

pinkavaj commented Nov 9, 2022

Btw I'm getting QAbstractItemView::closeEditor called with an editor that does not belong to this view when editing the tag name, any idea what is going on?

@vladisslav2011
Copy link
Contributor

I have no idea why this warning is emitted...
You have not picked the CI fix.

return false;

if (oldName == newName)
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 300..310 can be removed. I missed the same check at the line 254.
Or it may be better to remove check on line 254 as it looks better to compare trimmed strings.

@pinkavaj
Copy link
Contributor Author

pinkavaj commented Nov 9, 2022

You have not picked the CI fix.

The qDebug() is removed now.

It is more convenient for me to point at code rather than on the person, it usually creates less friction :)

@vladisslav2011
Copy link
Contributor

Sorry :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants