-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Track Info Dialog: Improve keyboard usability & resize behavior #13818
Open
cr7pt0gr4ph7
wants to merge
39
commits into
mixxxdj:main
Choose a base branch
from
cr7pt0gr4ph7:ready-for-merge/track-info-dialog
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
f81e8d2
DlgTrackInfo/LastPlayed: Add "Last played" field to track info dialog
cr7pt0gr4ph7 0eda4b9
DlgTrackInfo/LastPlayed: Display placeholder for last played date
cr7pt0gr4ph7 79b6d7f
DlgTrackInfo/Layout: Fix alignment of "Grouping" and "Comments" label
cr7pt0gr4ph7 c00dfc9
DlgTrackInfo/Cleanup: Fix: Make text of "Bitrate" field selectable
cr7pt0gr4ph7 9aa4ff0
DlgTrackInfo/Cleanup: Improve GroupBox names
cr7pt0gr4ph7 2e5a11b
DlgTrackInfo/Cleanup: Group related properties in the header
cr7pt0gr4ph7 9b9aa75
DlgTrackInfo/Cleanup: Simplify updateTrackMetadataFields
cr7pt0gr4ph7 f1f3089
DlgTrackInfo/Cleanup: Associate read-only fields with track property …
cr7pt0gr4ph7 25eb63c
WStarRating: Remove Qt5 compatibility code
cr7pt0gr4ph7 4b309a8
WStarRating: Add keyboard controls for modifying the star rating
cr7pt0gr4ph7 43ef74b
WStarRating: Highlight the control when having keyboard focus
cr7pt0gr4ph7 ae81d1f
DlgTrackInfo/StarRating: Move UI setup code into the .ui file.
cr7pt0gr4ph7 5bef6d2
DlgTrackInfo/StarRating: Integrate WStarRating into the tab order
cr7pt0gr4ph7 f60d4c2
DlgTrackInfo/LeftRight: Conditionally enable/disable btnNext and btnPrev
cr7pt0gr4ph7 ae797c2
DlgTrackInfo/LeftRight: Add Alt+Left / Alt+Right as additional shortc…
cr7pt0gr4ph7 643533e
DlgTrackInfo/LeftRight: Refocus the current widget when moving to a d…
cr7pt0gr4ph7 dcd791b
DlgTrackInfo/Sizing: Scroll to start of text fields by setting the cu…
cr7pt0gr4ph7 891d021
DlgTrackInfo/Sizing: Add WMultiLineTextEdit to allow for correct resi…
cr7pt0gr4ph7 3fba164
WMultiLineTextEdit: Style like QLineEdit (including the focus frame)
cr7pt0gr4ph7 fe5c1f9
DlgTrackInfo/Sizing: Add WBasicPushButton to elide text
cr7pt0gr4ph7 395697b
WBasicPushButton: Automatically generate tooltips when button is smal…
cr7pt0gr4ph7 4085793
DlgTrackInfo/UpDown: Change focus when pressing Up/Down on WBasicLine…
cr7pt0gr4ph7 239717d
DlgTrackInfo/UpDown: Change focus when pressing Up/Down on WMultiLine…
cr7pt0gr4ph7 c6a016d
DlgTrackInfo/UpDown: Change focus when pressing Up/Down on WStarRating
cr7pt0gr4ph7 c83f4cb
DlgTrackInfo/UpDown: Change focus when pressing Up/Down on WBasicTabW…
cr7pt0gr4ph7 c0ff995
DlgTrackInfo/UpDown: Change focus when pressing Up/Down on WBasicLabel
cr7pt0gr4ph7 a169351
DlgTrackInfo/UpDown: Sort WBasicLabel controls by visual & tab order
cr7pt0gr4ph7 5b2424d
DlgTrackInfo/ColorPicker: Avoid double-triggering the color picker popup
cr7pt0gr4ph7 d23e53d
WColorPicker: Fix oversight in idealColumnCount
cr7pt0gr4ph7 42f0b14
WColorPicker: Allow keyboard navigation in the color picker popup
cr7pt0gr4ph7 3589e57
WColorPicker: Set initially focused widget of color picker popup
cr7pt0gr4ph7 dbf2da0
DlgTrackInfo: Add tooltip for cover image
cr7pt0gr4ph7 8aa703f
Merge remote-tracking branch 'upstream/main' into ready-for-merge/tra…
cr7pt0gr4ph7 a5ae64e
DlgTrackInfo: Rename meatdata_buttons_layout to metadata_buttons_layout
cr7pt0gr4ph7 5ad242b
WColorPicker: Fix code formatting issue in WColorGridButton constructor
cr7pt0gr4ph7 25c005f
DlgTrackInfo/Cleanup: Associate "rating" and "color" property names w…
cr7pt0gr4ph7 0f3fefd
DlgTrackInfoMulti/StarRating: Integrate WStarRating into the tab order
cr7pt0gr4ph7 335c4cb
Use QKeyEvent::text() for handling Key_0..9
cr7pt0gr4ph7 7266f4d
Use 'const' and QStringLiteral where possible
cr7pt0gr4ph7 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
#include "library/dlgtrackinfo.h" | ||
|
||
#include <QShortcut> | ||
#include <QSignalBlocker> | ||
#include <QStyleFactory> | ||
#include <QtDebug> | ||
|
@@ -19,6 +20,7 @@ | |
#include "util/datetime.h" | ||
#include "util/desktophelper.h" | ||
#include "util/duration.h" | ||
#include "widget/wcolorpickeractionmenu.h" | ||
#include "widget/wcoverartlabel.h" | ||
#include "widget/wcoverartmenu.h" | ||
#include "widget/wstarrating.h" | ||
|
@@ -45,8 +47,7 @@ DlgTrackInfo::DlgTrackInfo( | |
m_tapFilter(this, kFilterLength, kMaxInterval), | ||
m_pWCoverArtMenu(make_parented<WCoverArtMenu>(this)), | ||
m_pWCoverArtLabel(make_parented<WCoverArtLabel>(this, m_pWCoverArtMenu)), | ||
m_pWStarRating(make_parented<WStarRating>(this)), | ||
m_pColorPicker(make_parented<WColorPickerAction>( | ||
m_pColorPicker(make_parented<WColorPickerActionMenu>( | ||
WColorPicker::Option::AllowNoColor | | ||
// TODO(xxx) remove this once the preferences are themed via QSS | ||
WColorPicker::Option::NoExtStyleSheet, | ||
|
@@ -55,38 +56,52 @@ DlgTrackInfo::DlgTrackInfo( | |
init(); | ||
} | ||
|
||
DlgTrackInfo::~DlgTrackInfo() { | ||
// ~parented_ptr() needs the definition of the wrapped type | ||
// upon deletion! Otherwise the behavior is undefined. | ||
// The wrapped types of some parented_ptr members are only | ||
// forward declared in the header file. | ||
} | ||
|
||
void DlgTrackInfo::init() { | ||
setupUi(this); | ||
setWindowIcon(QIcon(MIXXX_ICON_PATH)); | ||
|
||
// Store tag edit widget pointers to allow focusing a specific widgets when | ||
// this is opened by double-clicking a WTrackProperty label. | ||
// Associate with property strings taken from library/dao/trackdao.h | ||
// Associate with property strings taken from library/dao/trackdao.cpp | ||
m_propertyWidgets.insert("artist", txtArtist); | ||
m_propertyWidgets.insert("title", txtTrackName); | ||
m_propertyWidgets.insert("titleInfo", txtTrackName); | ||
m_propertyWidgets.insert("album", txtAlbum); | ||
m_propertyWidgets.insert("album_artist", txtAlbumArtist); | ||
m_propertyWidgets.insert("composer", txtComposer); | ||
m_propertyWidgets.insert("genre", txtGenre); | ||
m_propertyWidgets.insert("rating", starRating); | ||
m_propertyWidgets.insert("year", txtYear); | ||
m_propertyWidgets.insert("bpm", spinBpm); | ||
m_propertyWidgets.insert("tracknumber", txtTrackNumber); | ||
m_propertyWidgets.insert("key", txtKey); | ||
m_propertyWidgets.insert("grouping", txtGrouping); | ||
m_propertyWidgets.insert("comment", txtComment); | ||
m_propertyWidgets.insert("color", btnColorPicker); | ||
m_propertyWidgets.insert("datetime_added", txtDateAdded); | ||
m_propertyWidgets.insert("duration", txtDuration); | ||
m_propertyWidgets.insert("timesplayed", txtDateLastPlayed); | ||
m_propertyWidgets.insert("last_played_at", txtDateLastPlayed); | ||
m_propertyWidgets.insert("filetype", txtType); | ||
m_propertyWidgets.insert("bitrate", txtBitrate); | ||
m_propertyWidgets.insert("samplerate", txtSamplerate); | ||
m_propertyWidgets.insert("replaygain", txtReplayGain); | ||
m_propertyWidgets.insert("replaygain_peak", txtReplayGain); | ||
m_propertyWidgets.insert("track_locations.location", txtLocation); | ||
|
||
coverLayout->setAlignment(Qt::AlignRight | Qt::AlignTop); | ||
coverLayout->setSpacing(0); | ||
coverLayout->setContentsMargins(0, 0, 0, 0); | ||
coverLayout->insertWidget(0, m_pWCoverArtLabel.get()); | ||
|
||
starsLayout->setAlignment(Qt::AlignRight | Qt::AlignVCenter); | ||
starsLayout->setSpacing(0); | ||
starsLayout->setContentsMargins(0, 0, 0, 0); | ||
starsLayout->insertWidget(0, m_pWStarRating.get()); | ||
// This is necessary to pass on mouseMove events to WStarRating | ||
m_pWStarRating->setMouseTracking(true); | ||
// Workaround: Align the baseline of the "Comments" label | ||
// with the baseline of the text inside the comments field | ||
const int topMargin = txtComment->frameWidth() + int(txtComment->document()->documentMargin()); | ||
lblTrackComment->setContentsMargins(0, topMargin, 0, 0); | ||
|
||
if (m_pTrackModel) { | ||
connect(btnNext, | ||
|
@@ -97,6 +112,18 @@ void DlgTrackInfo::init() { | |
&QPushButton::clicked, | ||
this, | ||
&DlgTrackInfo::slotPrevButton); | ||
|
||
QShortcut* nextShortcut = new QShortcut(QKeySequence("Alt+Right"), this); | ||
QShortcut* prevShortcut = new QShortcut(QKeySequence("Alt+Left"), this); | ||
|
||
connect(nextShortcut, | ||
&QShortcut::activated, | ||
btnNext, | ||
[this] { btnNext->animateClick(); }); | ||
connect(prevShortcut, | ||
&QShortcut::activated, | ||
btnPrev, | ||
[this] { btnPrev->animateClick(); }); | ||
} else { | ||
btnNext->hide(); | ||
btnPrev->hide(); | ||
|
@@ -248,6 +275,10 @@ void DlgTrackInfo::init() { | |
&DlgTrackInfo::slotOpenInFileBrowser); | ||
|
||
// Cover art | ||
m_pWCoverArtLabel->setToolTip( | ||
tr("Left-click to show larger preview") + "\n" + | ||
tr("Right-click for more options")); | ||
Comment on lines
+278
to
+280
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's move this to WCoverArtLabel so it hasto be translated only once. |
||
|
||
CoverArtCache* pCache = CoverArtCache::instance(); | ||
if (pCache) { | ||
connect(pCache, | ||
|
@@ -266,22 +297,16 @@ void DlgTrackInfo::init() { | |
this, | ||
&DlgTrackInfo::slotReloadCoverArt); | ||
|
||
connect(m_pWStarRating, | ||
connect(starRating, | ||
&WStarRating::ratingChangeRequest, | ||
this, | ||
&DlgTrackInfo::slotRatingChanged); | ||
|
||
btnColorPicker->setStyle(QStyleFactory::create(QStringLiteral("fusion"))); | ||
QMenu* pColorPickerMenu = new QMenu(this); | ||
pColorPickerMenu->addAction(m_pColorPicker); | ||
btnColorPicker->setMenu(pColorPickerMenu); | ||
btnColorPicker->setMenu(m_pColorPicker.get()); | ||
|
||
connect(btnColorPicker, | ||
&QPushButton::clicked, | ||
this, | ||
&DlgTrackInfo::slotColorButtonClicked); | ||
connect(m_pColorPicker.get(), | ||
&WColorPickerAction::colorPicked, | ||
&WColorPickerActionMenu::colorPicked, | ||
this, | ||
[this](const mixxx::RgbColor::optional_t& newColor) { | ||
trackColorDialogSetColor(newColor); | ||
|
@@ -320,24 +345,45 @@ void DlgTrackInfo::slotPrevDlgTagFetcher() { | |
loadPrevTrack(); | ||
} | ||
|
||
QModelIndex DlgTrackInfo::getPrevNextTrack(bool next) { | ||
return m_currentTrackIndex.sibling( | ||
m_currentTrackIndex.row() + (next ? 1 : -1), | ||
m_currentTrackIndex.column()); | ||
} | ||
|
||
void DlgTrackInfo::loadNextTrack() { | ||
auto nextRow = m_currentTrackIndex.sibling( | ||
m_currentTrackIndex.row() + 1, m_currentTrackIndex.column()); | ||
auto nextRow = getPrevNextTrack(true); | ||
if (nextRow.isValid()) { | ||
loadTrack(nextRow); | ||
refocusCurrentWidget(); | ||
emit next(); | ||
} | ||
} | ||
|
||
void DlgTrackInfo::loadPrevTrack() { | ||
QModelIndex prevRow = m_currentTrackIndex.sibling( | ||
m_currentTrackIndex.row() - 1, m_currentTrackIndex.column()); | ||
auto prevRow = getPrevNextTrack(false); | ||
if (prevRow.isValid()) { | ||
loadTrack(prevRow); | ||
refocusCurrentWidget(); | ||
emit previous(); | ||
} | ||
} | ||
|
||
/// Simulate moving the focus out of and then back into the currently | ||
/// focused widget to trigger behaviors like the "Select all on focus" | ||
/// for QLineEdit. | ||
/// | ||
/// This is done when moving to the previous/next track, because, | ||
/// logically, the user has moved to a new dialog, even though we | ||
/// reuse the current dialog instance internally. | ||
void DlgTrackInfo::refocusCurrentWidget() { | ||
QWidget* focusedWidget = QApplication::focusWidget(); | ||
if (focusedWidget && isAncestorOf(focusedWidget)) { | ||
focusedWidget->clearFocus(); | ||
focusedWidget->setFocus(Qt::ShortcutFocusReason); | ||
} | ||
} | ||
|
||
void DlgTrackInfo::updateFromTrack(const Track& track) { | ||
const QSignalBlocker signalBlocker(this); | ||
|
||
|
@@ -355,7 +401,7 @@ void DlgTrackInfo::updateFromTrack(const Track& track) { | |
|
||
reloadTrackBeats(track); | ||
|
||
m_pWStarRating->slotSetRating(m_pLoadedTrack->getRating()); | ||
starRating->slotSetRating(m_pLoadedTrack->getRating()); | ||
} | ||
|
||
void DlgTrackInfo::replaceTrackRecord( | ||
|
@@ -378,50 +424,60 @@ void DlgTrackInfo::replaceTrackRecord( | |
mixxx::displayLocalDateTime( | ||
mixxx::localDateTimeFromUtc( | ||
m_trackRecord.getDateAdded()))); | ||
auto lastPlayed = m_trackRecord.getPlayCounter().getLastPlayedAt(); | ||
txtDateLastPlayed->setText(lastPlayed.isValid() | ||
? mixxx::displayLocalDateTime(mixxx::localDateTimeFromUtc(lastPlayed)) | ||
: QStringLiteral("-")); | ||
|
||
updateTrackMetadataFields(); | ||
} | ||
|
||
void DlgTrackInfo::updateTrackMetadataFields() { | ||
const auto metadata = m_trackRecord.getMetadata(); | ||
const auto trackInfo = metadata.getTrackInfo(); | ||
const auto albumInfo = metadata.getAlbumInfo(); | ||
const auto signalInfo = metadata.getStreamInfo().getSignalInfo(); | ||
|
||
// Editable fields | ||
txtTrackName->setText( | ||
m_trackRecord.getMetadata().getTrackInfo().getTitle()); | ||
txtArtist->setText( | ||
m_trackRecord.getMetadata().getTrackInfo().getArtist()); | ||
txtAlbum->setText( | ||
m_trackRecord.getMetadata().getAlbumInfo().getTitle()); | ||
txtAlbumArtist->setText( | ||
m_trackRecord.getMetadata().getAlbumInfo().getArtist()); | ||
txtGenre->setText( | ||
m_trackRecord.getMetadata().getTrackInfo().getGenre()); | ||
txtComposer->setText( | ||
m_trackRecord.getMetadata().getTrackInfo().getComposer()); | ||
txtGrouping->setText( | ||
m_trackRecord.getMetadata().getTrackInfo().getGrouping()); | ||
txtYear->setText( | ||
m_trackRecord.getMetadata().getTrackInfo().getYear()); | ||
txtTrackNumber->setText( | ||
m_trackRecord.getMetadata().getTrackInfo().getTrackNumber()); | ||
txtComment->setPlainText( | ||
m_trackRecord.getMetadata().getTrackInfo().getComment()); | ||
txtBpm->setText( | ||
m_trackRecord.getMetadata().getTrackInfo().getBpmText()); | ||
txtTrackName->setText(trackInfo.getTitle()); | ||
txtArtist->setText(trackInfo.getArtist()); | ||
txtAlbum->setText(albumInfo.getTitle()); | ||
txtAlbumArtist->setText(albumInfo.getArtist()); | ||
txtGenre->setText(trackInfo.getGenre()); | ||
txtComposer->setText(trackInfo.getComposer()); | ||
txtGrouping->setText(trackInfo.getGrouping()); | ||
txtYear->setText(trackInfo.getYear()); | ||
txtTrackNumber->setText(trackInfo.getTrackNumber()); | ||
txtComment->setPlainText(trackInfo.getComment()); | ||
txtBpm->setText(trackInfo.getBpmText()); | ||
displayKeyText(); | ||
|
||
// Set cursor / scroll position of editable fields (only relevant | ||
// when the content's width is larger than the width of the QLineEdit) | ||
txtTrackName->setCursorPosition(0); | ||
txtArtist->setCursorPosition(0); | ||
txtAlbum->setCursorPosition(0); | ||
txtAlbumArtist->setCursorPosition(0); | ||
txtGenre->setCursorPosition(0); | ||
txtComposer->setCursorPosition(0); | ||
txtGrouping->setCursorPosition(0); | ||
txtYear->setCursorPosition(0); | ||
txtTrackNumber->setCursorPosition(0); | ||
|
||
// Non-editable fields | ||
txtDuration->setText( | ||
m_trackRecord.getMetadata().getDurationText(mixxx::Duration::Precision::SECONDS)); | ||
QString bitrate = m_trackRecord.getMetadata().getBitrateText(); | ||
metadata.getDurationText(mixxx::Duration::Precision::SECONDS)); | ||
QString bitrate = metadata.getBitrateText(); | ||
if (bitrate.isEmpty()) { | ||
txtBitrate->clear(); | ||
} else { | ||
txtBitrate->setText(bitrate + QChar(' ') + mixxx::audio::Bitrate::unit()); | ||
} | ||
txtReplayGain->setText( | ||
mixxx::ReplayGain::ratioToString( | ||
m_trackRecord.getMetadata().getTrackInfo().getReplayGain().getRatio())); | ||
trackInfo.getReplayGain().getRatio())); | ||
|
||
auto samplerate = m_trackRecord.getMetadata().getStreamInfo().getSignalInfo().getSampleRate(); | ||
auto samplerate = signalInfo.getSampleRate(); | ||
if (samplerate.isValid()) { | ||
txtSamplerate->setText(QString::number(samplerate.value()) + " Hz"); | ||
} else { | ||
|
@@ -493,7 +549,11 @@ void DlgTrackInfo::loadTrack(const QModelIndex& index) { | |
return; | ||
} | ||
TrackPointer pTrack = m_pTrackModel->getTrack(index); | ||
|
||
m_currentTrackIndex = index; | ||
btnPrev->setEnabled(getPrevNextTrack(false).isValid()); | ||
btnNext->setEnabled(getPrevNextTrack(true).isValid()); | ||
|
||
loadTrackInternal(pTrack); | ||
if (m_pDlgTagFetcher && m_pDlgTagFetcher->isVisible()) { | ||
m_pDlgTagFetcher->loadTrack(m_currentTrackIndex); | ||
|
@@ -547,13 +607,6 @@ void DlgTrackInfo::slotOpenInFileBrowser() { | |
mixxx::DesktopHelper::openInFileBrowser(QStringList(m_pLoadedTrack->getLocation())); | ||
} | ||
|
||
void DlgTrackInfo::slotColorButtonClicked() { | ||
if (!m_pLoadedTrack) { | ||
return; | ||
} | ||
btnColorPicker->showMenu(); | ||
} | ||
|
||
void DlgTrackInfo::trackColorDialogSetColor(const mixxx::RgbColor::optional_t& newColor) { | ||
m_pColorPicker->setSelectedColor(newColor); | ||
btnColorPicker->menu()->close(); | ||
|
@@ -638,7 +691,7 @@ void DlgTrackInfo::clear() { | |
|
||
txtLocation->setText(""); | ||
|
||
m_pWStarRating->slotSetRating(0); | ||
starRating->slotSetRating(0); | ||
} | ||
|
||
void DlgTrackInfo::slotBpmScale(mixxx::Beats::BpmScale bpmScale) { | ||
|
@@ -753,7 +806,7 @@ void DlgTrackInfo::slotRatingChanged(int rating) { | |
} | ||
if (m_trackRecord.isValidRating(rating) && | ||
rating != m_trackRecord.getRating()) { | ||
m_pWStarRating->slotSetRating(rating); | ||
starRating->slotSetRating(rating); | ||
m_trackRecord.setRating(rating); | ||
} | ||
} | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of the QWidget* list is being able to focus editable fields.
What's the benefit of adding the QLabels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WTrackProperty
can be used by skins for arbitrary properties, including the non-editable ones. So, basically, just for completeness sake & to provide uniform behavior for those properties, too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm AFAIU non-editable fileds (labels) can't be focused anyway, so wouldn't the UI state be somewhat unclear then?
I mean, we wouldn't know the current focus widget/position until we press Tab/Shift+Tab, right?