Skip to content

#454 Allow to move tabs #573

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 4 commits into from
Feb 24, 2021
Merged

#454 Allow to move tabs #573

merged 4 commits into from
Feb 24, 2021

Conversation

asashnov
Copy link
Contributor

@asashnov asashnov commented Jan 6, 2021

Fixes #454

Public interface of tabbar.h changes:

  • currentWidget() -> currentZimView() (because the method returned nullptr if library or setting page is current)
  • widget(int) illiminated, replaced with closeTabsByZimId(id) because this is what it needed for;

TabBar Implementation changes:

  • m_settingsIndex removed, not needed. Replaced to the usage of qobject_cast<> to determine the type of a specific tab widget, instead of trying to track it;
  • fixed hardcoded conditions that the index 0 is the library page (it also can be moved);
  • remove unused mp_contentManagerView property

Added Q_OBJECT macros in classes ContentManagerView and SettingsManagerView, otherwise qobject_cast doesn't work (https://wiki.qt.io/Coding_Conventions).

@kelson42 kelson42 requested a review from mgautierfr January 6, 2021 20:47
@asashnov
Copy link
Contributor Author

asashnov commented Jan 6, 2021

The current implementation works, but has a bug:

Steps to reproduce:

  • Run the app
  • open Settings
  • move Settings tab to the left (to the place of the library)
  • open new tab (by +)

Actual result: Settings tab will be collapsed to the icon

@kelson42 kelson42 removed the request for review from mgautierfr January 7, 2021 07:21
@kelson42 kelson42 marked this pull request as draft January 7, 2021 09:47
@kelson42
Copy link
Collaborator

kelson42 commented Jan 7, 2021

@asashnov Have put the PR to draft, so i can be polished before review.

@stale
Copy link

stale bot commented Jan 15, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added stale and removed stale labels Jan 15, 2021
@asashnov asashnov force-pushed the 454-moving-tabs branch 2 times, most recently from e367deb to ad31879 Compare January 17, 2021 17:31
@asashnov asashnov marked this pull request as ready for review January 17, 2021 17:44
@asashnov asashnov requested a review from mgautierfr January 17, 2021 17:44
@asashnov asashnov changed the title WIP #454 Allow to move tabs #454 Allow to move tabs Jan 17, 2021
@kelson42 kelson42 self-requested a review January 18, 2021 11:30
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

@asashnov Works almost perfectly to my opinion. Only one point. Can you please make the library tab stick to the left (so effectively forbids to move a tab before)?

Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

Sticky library tab works perfectly. Thx!

@kelson42
Copy link
Collaborator

Have just rebased the branch.

@stale
Copy link

stale bot commented Feb 7, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Feb 7, 2021
@stale stale bot removed the stale label Feb 8, 2021
@asashnov asashnov requested a review from kelson42 February 8, 2021 11:46
@asashnov
Copy link
Contributor Author

asashnov commented Feb 8, 2021

The branch rebased on top of current master (d140601), multiple commits squashed to one commit.

Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

Code is good.

It is matter of taste/style but I prefer to use auto instead of being explicit on the type of the object.

The same way, I prefer

auto zv = currentZimView();
if (zv) {
    return zv->getWebView();
}
return nullptr;

to

if (ZimView *zv = currentZimView())
    return zv->getWebView();
return nullptr;

Also, I like to have a PR split is several logical commits (but squash commits fixing errors in previous commits of the same PR)

But I will not block the PR for that.
Code is good and functional.

@asashnov
Copy link
Contributor Author

asashnov commented Feb 12, 2021

Replaced to auto in lines:

    } else if (ZimView *zv = qobject_cast<ZimView*>(w)) {

    ZimView *zv = qobject_cast<ZimView*>(mp_stackedWidget->widget(i));

because pointer type is clearly noted by qobject_cast.

In other cases we should be clear about what we want to get from a tab: QWidget, WebView or ZimView.

Splitted the change to 4 logical commits.

@asashnov asashnov force-pushed the 454-moving-tabs branch 3 times, most recently from fec0f8c to 933491b Compare February 12, 2021 19:58
@asashnov
Copy link
Contributor Author

Fixed compilation for all 4 commits after splitting the change.

@kelson42
Copy link
Collaborator

@asashnov Would you please rebase on master?

Fix potential segfault when settings tab is open and closing by zim ID
* the Library tab isn't necessary the first tab;
* moving the Library tab is possible, but prohibited in TabBar::onTabMoved()
* Tabbar uses qobject_cast<> to recognize tab types
  instead of previously used settings and library (always 0) tab indexes.
@asashnov
Copy link
Contributor Author

@kelson42 rebased and tested, ready to merge.

@kelson42 kelson42 merged commit 0183274 into master Feb 24, 2021
@kelson42 kelson42 deleted the 454-moving-tabs branch February 24, 2021 12:02
@mgautierfr mgautierfr mentioned this pull request May 4, 2021
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.

Moving tabs in Kiwix
3 participants