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

rename FxLine directly in a QLineEdit #2918

Merged
merged 6 commits into from Aug 7, 2016

Conversation

Projects
None yet
6 participants
@BaraMGB
Contributor

BaraMGB commented Jul 15, 2016

Same as #2916. For avoiding dialog window we can use a QLineEdit for renaming the FxLine. I used a QGraphicsView for rotating the QLineEdit.

renamefxline

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 17, 2016

Member

The beginning of the qlinedit is higher than the text's original position, which seems inconsistent to me. Other than that, looks good and avoids dialogs, so I'm all for that.

Member

Umcaruje commented Jul 17, 2016

The beginning of the qlinedit is higher than the text's original position, which seems inconsistent to me. Other than that, looks good and avoids dialogs, so I'm all for that.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 17, 2016

Member

Can someone explain the purpose of using two components over one in this instance? For example, can't we simply keep the input a QLineEdit and change the behavior depending on whether or not we're in edit mode by setting readOnly to true?

Member

tresf commented Jul 17, 2016

Can someone explain the purpose of using two components over one in this instance? For example, can't we simply keep the input a QLineEdit and change the behavior depending on whether or not we're in edit mode by setting readOnly to true?

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 17, 2016

Member

Can someone explain the purpose of using two components over one in this instance? For example, can't we simply keep the input a QLineEdit and change the behavior depending on whether or not we're in edit mode by setting readOnly to true?

Well, if one can make the disabled QLineEdit completely backgroundless and borderless, then that would be a better solution.

Member

Umcaruje commented Jul 17, 2016

Can someone explain the purpose of using two components over one in this instance? For example, can't we simply keep the input a QLineEdit and change the behavior depending on whether or not we're in edit mode by setting readOnly to true?

Well, if one can make the disabled QLineEdit completely backgroundless and borderless, then that would be a better solution.

@BaraMGB

This comment has been minimized.

Show comment
Hide comment
@BaraMGB

BaraMGB Jul 22, 2016

Contributor

Update: Now it uses for both (labeling and renaming) a QLineEdit.

Contributor

BaraMGB commented Jul 22, 2016

Update: Now it uses for both (labeling and renaming) a QLineEdit.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 24, 2016

Member

screenshot from 2016-07-24 14 59 22

Tested, looks and works like a charm 👍

Member

Umcaruje commented Jul 24, 2016

screenshot from 2016-07-24 14 59 22

Tested, looks and works like a charm 👍

Show outdated Hide outdated src/gui/widgets/FxLine.cpp
m_scene->setSceneRect( 0, 0, 33, FxLineHeight );
m_view = new QGraphicsView( this );
m_view->setStyleSheet( "border-style: none; background: transparent;" );

This comment has been minimized.

@Umcaruje

Umcaruje Jul 24, 2016

Member

Shouldn't you do this in the CSS?

@Umcaruje

Umcaruje Jul 24, 2016

Member

Shouldn't you do this in the CSS?

This comment has been minimized.

@BaraMGB

BaraMGB Jul 24, 2016

Contributor

I don't think so. The QGraphicsView is only a container for the QLineEdit. With it we can rotate the QLineEdit. I can't imagine a situation where the view should have borders or a background color. It's an internal solution the user (themer) don't need to know.

@BaraMGB

BaraMGB Jul 24, 2016

Contributor

I don't think so. The QGraphicsView is only a container for the QLineEdit. With it we can rotate the QLineEdit. I can't imagine a situation where the view should have borders or a background color. It's an internal solution the user (themer) don't need to know.

This comment has been minimized.

@Umcaruje

Umcaruje Jul 24, 2016

Member

Ah, ok, thanks for clarifying 👍. This looks good to merge.

@Umcaruje

Umcaruje Jul 24, 2016

Member

Ah, ok, thanks for clarifying 👍. This looks good to merge.

This comment has been minimized.

@midi-pascal

midi-pascal Jul 24, 2016

Contributor

Just to let you know: if a track or a fx is renamed Lmms does not consider the project as modified.
You can quit with no question about saving...

@midi-pascal

midi-pascal Jul 24, 2016

Contributor

Just to let you know: if a track or a fx is renamed Lmms does not consider the project as modified.
You can quit with no question about saving...

Show outdated Hide outdated include/FxLine.h
FxMixerView * m_mv;
FxMixer * m_mix;

This comment has been minimized.

@jasp00

jasp00 Jul 24, 2016

Member

Why this member? There is only one FxMixer.

@jasp00

jasp00 Jul 24, 2016

Member

Why this member? There is only one FxMixer.

Show outdated Hide outdated src/gui/widgets/FxLine.cpp
@@ -23,22 +23,20 @@
*
*/
#include "FxLine.h"

This comment has been minimized.

@jasp00

jasp00 Jul 24, 2016

Member

This #include should not be reordered. Besides being a coding convention, it checks that the header has all its dependencies declared.

@jasp00

jasp00 Jul 24, 2016

Member

This #include should not be reordered. Besides being a coding convention, it checks that the header has all its dependencies declared.

Show outdated Hide outdated src/gui/widgets/FxLine.cpp
m_renameLineEdit->setFixedWidth( 65 );
m_lcd->show();
m_newName = m_renameLineEdit->text();
this->setFocus();

This comment has been minimized.

@jasp00

jasp00 Jul 24, 2016

Member

Why this->?

@jasp00

jasp00 Jul 24, 2016

Member

Why this->?

@BaraMGB

This comment has been minimized.

Show comment
Hide comment
@BaraMGB

BaraMGB Jul 24, 2016

Contributor

@jasp00 Thanks for your revisiting. I updated the branch.
@midi-pascal interesting. I have to figure out how to fix that. perhaps you file an issue for that.

Contributor

BaraMGB commented Jul 24, 2016

@jasp00 Thanks for your revisiting. I updated the branch.
@midi-pascal interesting. I have to figure out how to fix that. perhaps you file an issue for that.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 24, 2016

Member

I have to figure out how to fix that. perhaps you file an issue for that.

I think you just add Engine::getSong()->setModified(); after your changes.

Member

zonkmachine commented Jul 24, 2016

I have to figure out how to fix that. perhaps you file an issue for that.

I think you just add Engine::getSong()->setModified(); after your changes.

@jasp00

This comment has been minimized.

Show comment
Hide comment
@jasp00

jasp00 Jul 24, 2016

Member

Undo and redo should work with renaming. A StringModel should be implemented.

Member

jasp00 commented Jul 24, 2016

Undo and redo should work with renaming. A StringModel should be implemented.

@BaraMGB

This comment has been minimized.

Show comment
Hide comment
@BaraMGB

BaraMGB Jul 25, 2016

Contributor

I think you just add Engine::getSong()->setModified(); after your changes.

@zonkmachine works!

Undo and redo should work with renaming. A StringModel should be implemented.

@jasp00 can you point me to some code where I can learn how to do this?

Contributor

BaraMGB commented Jul 25, 2016

I think you just add Engine::getSong()->setModified(); after your changes.

@zonkmachine works!

Undo and redo should work with renaming. A StringModel should be implemented.

@jasp00 can you point me to some code where I can learn how to do this?

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 25, 2016

Member

The model classes are generated dynamically, si they're a bit hard to find in the code, but search for IntModel for an example of an Integer version.

Member

tresf commented Jul 25, 2016

The model classes are generated dynamically, si they're a bit hard to find in the code, but search for IntModel for an example of an Integer version.

@BaraMGB

This comment has been minimized.

Show comment
Hide comment
@BaraMGB

BaraMGB Jul 26, 2016

Contributor

I have updated. Now the project is set modified if the FxLine is renamed. But I fear I have no clue how to handle the undo/redo stuff. 😟

Contributor

BaraMGB commented Jul 26, 2016

I have updated. Now the project is set modified if the FxLine is renamed. But I fear I have no clue how to handle the undo/redo stuff. 😟

@jasp00

This comment has been minimized.

Show comment
Hide comment
@jasp00

jasp00 Jul 26, 2016

Member

But I fear I have no clue how to handle the undo/redo stuff.

That can be done later.

Member

jasp00 commented Jul 26, 2016

But I fear I have no clue how to handle the undo/redo stuff.

That can be done later.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 27, 2016

Member

Tested this one out too, works like a charm. Here's a GIF:
gifrecord_2016-07-27_191956

Member

Umcaruje commented Jul 27, 2016

Tested this one out too, works like a charm. Here's a GIF:
gifrecord_2016-07-27_191956

Show outdated Hide outdated src/gui/widgets/FxLine.cpp
}
m_renameLineEdit->setText( elideName( m_newName ) );
Engine::getSong()->setModified();

This comment has been minimized.

@jasp00

jasp00 Jul 28, 2016

Member

setModified() should be called only if text has changed.

@jasp00

jasp00 Jul 28, 2016

Member

setModified() should be called only if text has changed.

@BaraMGB

This comment has been minimized.

Show comment
Hide comment
@BaraMGB

BaraMGB Jul 28, 2016

Contributor

So, now it should works better.

Contributor

BaraMGB commented Jul 28, 2016

So, now it should works better.

Show outdated Hide outdated include/FxLine.h
QStaticText m_staticTextName;
bool m_inRename;
QString m_newName;

This comment has been minimized.

@jasp00

jasp00 Aug 1, 2016

Member

It looks like this member is not needed. A local variable newName would be better.

@jasp00

jasp00 Aug 1, 2016

Member

It looks like this member is not needed. A local variable newName would be better.

Show outdated Hide outdated include/FxLine.h
QLineEdit * m_renameLineEdit;
QGraphicsProxyWidget * m_proxyWidget;
QGraphicsView * m_view;
QGraphicsScene * m_scene;

This comment has been minimized.

@jasp00

jasp00 Aug 1, 2016

Member

m_proxyWidget and m_scene do not need to be accessible as members.

@jasp00

jasp00 Aug 1, 2016

Member

m_proxyWidget and m_scene do not need to be accessible as members.

Show outdated Hide outdated include/FxLine.h
QGraphicsProxyWidget * m_proxyWidget;
QGraphicsView * m_view;
QGraphicsScene * m_scene;
QString m_name;

This comment has been minimized.

@jasp00

jasp00 Aug 1, 2016

Member

This member is not needed. It is set to Engine::fxMixer()->effectChannel( m_channelIndex )->m_name.

@jasp00

jasp00 Aug 1, 2016

Member

This member is not needed. It is set to Engine::fxMixer()->effectChannel( m_channelIndex )->m_name.

Show outdated Hide outdated src/gui/widgets/FxLine.cpp
QString FxLine::elideName( QString name )
{
const int maxTextHeight = 70;
QFontMetrics metrics( FxLine::font() );

This comment has been minimized.

@jasp00

jasp00 Aug 1, 2016

Member

Why FxLine::font() instead of font()?

@jasp00

jasp00 Aug 1, 2016

Member

Why FxLine::font() instead of font()?

@BaraMGB

This comment has been minimized.

Show comment
Hide comment
@BaraMGB

BaraMGB Aug 2, 2016

Contributor

Update. @jasp00 thank you very much for your revisiting!

Contributor

BaraMGB commented Aug 2, 2016

Update. @jasp00 thank you very much for your revisiting!

Show outdated Hide outdated src/gui/widgets/FxLine.cpp
m_renameLineEdit = new QLineEdit();
m_renameLineEdit->setText( name );
m_renameLineEdit->setFixedWidth( 65 );
m_renameLineEdit->setFont( pointSizeF( FxLine::font(), 7.5f ) );

This comment has been minimized.

@jasp00

jasp00 Aug 4, 2016

Member

Just one FxLine::font() more.

@jasp00

jasp00 Aug 4, 2016

Member

Just one FxLine::font() more.

Show outdated Hide outdated src/gui/widgets/FxLine.cpp
drawFxLine( &painter, this,
mix->effectChannel( m_channelIndex )->m_name,
m_mv->currentFxLine() == this, sendToThis, receiveFromThis );
drawFxLine( &painter, this, m_mv->currentFxLine() == this, sendToThis, receiveFromThis );

This comment has been minimized.

@jasp00

jasp00 Aug 4, 2016

Member

If you want to keep one line, you may change the tab before m_mv into a space.

@jasp00

jasp00 Aug 4, 2016

Member

If you want to keep one line, you may change the tab before m_mv into a space.

@BaraMGB

This comment has been minimized.

Show comment
Hide comment
@BaraMGB

BaraMGB Aug 4, 2016

Contributor

👍

Contributor

BaraMGB commented Aug 4, 2016

👍

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Aug 4, 2016

Member

Cool!

One comment. When you hover over the text ( shows horizontally ) the string is remembered.
If you now change the text and hover over it again it's the previous line that shows up.
Edit: Before you hit enter that is....

Member

zonkmachine commented Aug 4, 2016

Cool!

One comment. When you hover over the text ( shows horizontally ) the string is remembered.
If you now change the text and hover over it again it's the previous line that shows up.
Edit: Before you hit enter that is....

Show outdated Hide outdated include/FxLine.h
static const int FxLineHeight;
private:
void drawFxLine( QPainter* p, const FxLine *fxLine, const QString& name, bool isActive, bool sendToThis, bool receiveFromThis );
void drawFxLine( QPainter* p, const FxLine *fxLine, bool isActive, bool sendToThis, bool receiveFromThis );
QString elideName( QString name );

This comment has been minimized.

@jasp00

jasp00 Aug 5, 2016

Member

const QString & name should be used instead of QString name to avoid the copy of the QString.

@jasp00

jasp00 Aug 5, 2016

Member

const QString & name should be used instead of QString name to avoid the copy of the QString.

Show outdated Hide outdated src/gui/widgets/FxLine.cpp
QString name = Engine::fxMixer()->effectChannel( m_channelIndex )->m_name;
if( !m_inRename && m_renameLineEdit->text() != elideName( name ) )
{
m_renameLineEdit->setText( elideName( name ) );

This comment has been minimized.

@jasp00

jasp00 Aug 5, 2016

Member

You may want to cache elideName( name ) to avoid this second call.

@jasp00

jasp00 Aug 5, 2016

Member

You may want to cache elideName( name ) to avoid this second call.

@BaraMGB

This comment has been minimized.

Show comment
Hide comment
@BaraMGB

BaraMGB Aug 5, 2016

Contributor

@jasp00 update. 👍
@zonkmachine I turned the tool tip off in rename mode now. Good catch!

Contributor

BaraMGB commented Aug 5, 2016

@jasp00 update. 👍
@zonkmachine I turned the tool tip off in rename mode now. Good catch!

@jasp00 jasp00 merged commit a72ddf0 into LMMS:master Aug 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment