Skip to content

Show a useful icon in SQL tabs #2153

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

Merged
merged 2 commits into from
May 2, 2020
Merged

Show a useful icon in SQL tabs #2153

merged 2 commits into from
May 2, 2020

Conversation

mgrojo
Copy link
Member

@mgrojo mgrojo commented Mar 2, 2020

The icon shows whether the tab is linked to a project file (tab icon) or
to a file in disk (document_open icon).

This has a lateral effect of fixing this, since the tab always has an icon
and the height of the tab bar never changes:

#2073 (comment)

I had this change in my local repository and don't want to merge it so close to the release without some feedback. The code changes are minimal, but I'm not sure about whether we want icons there, or what icons should be used. For #2073 there is probably other solution, but I was thinking some time ago that it would be nice to have a visual way to know whether the tab is saved in a file or in the project file. At some time we might also want to have SQL files saved in the project file as a reference to an external file, and not as a saved text embedded in the project.

Copy link
Member

@MKleusberg MKleusberg left a comment

Choose a reason for hiding this comment

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

Other than these two minor points I don't see any problem with this. We might want to add two extra icons for this purpose though because I think if I didn't know about the meaning of these icons I wouldn't have guessed it correctly. On the other hand I couldn't find better icons either 😉 So feel free to merge this with the current icons - fixing a bug is more important than good looking icons I guess 😄

Comment on lines 346 to 347
//QIcon execIcon(":icons/hourglass");
ui->tabSqlAreas->setMinimumSize(ui->tabSqlAreas->iconSize());
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, sorry. A test leftover.

Comment on lines 1207 to 1208
// NOTE It's a bit hack-ish but we don't use this icon just as a signal to the user but also check for it in various places to check whether a
// specific SQL tab is currently running a query or not.
Copy link
Member

@MKleusberg MKleusberg Mar 10, 2020

Choose a reason for hiding this comment

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

Can you remove that comment, now that it is outdated? Or better yet, update it to explain that the tab data value means 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course. I'll update.

The icon shows whether the tab is linked to a project file (open_sql icon)
or to a file in disk (document_open icon).

This has a lateral effect of fixing this, since the tab always has an icon
and the height of the tab bar never changes:

#2073 (comment)
@mgrojo
Copy link
Member Author

mgrojo commented Mar 12, 2020

@MKleusberg I've fixed the observations and I propose another icon, which we already had but without use.

@mgrojo mgrojo merged commit 81b9b5a into master May 2, 2020
@mgrojo
Copy link
Member Author

mgrojo commented May 2, 2020

I've merged this to master, since the observations had been fixed and now we have a 3.12.x version and the decision to merge it to that release can be taken later.

@mgrojo mgrojo deleted the sql_tab_icons branch May 2, 2020 22:49
@MKleusberg
Copy link
Member

Thanks, @mgrojo! And sorry for the delay on my side 😇

@mgrojo
Copy link
Member Author

mgrojo commented May 6, 2020

Don't worry! You had already reviewed it. Should we consider it as candidate for 3.12.x? I fear spartan users who never seem to have enough space, though 😉

@MKleusberg
Copy link
Member

Yeah, let's include it in 3.12.x because besides being more useful it somewhat fixes the multithreading code we introduce in that release 😄

justinclift pushed a commit that referenced this pull request Jun 6, 2020
The icon shows whether the tab is linked to a project file (open_sql icon)
or to a file in disk (document_open icon).

This has a lateral effect of fixing this, since the tab always has an icon
and the height of the tab bar never changes:

#2073 (comment)
@justinclift justinclift added this to the 3.12.0 - Next release milestone Jun 8, 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.

3 participants