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

Loading a file and exiting will change "open recent" file order #1093

Closed
Jose-Moreno opened this Issue Oct 5, 2018 · 12 comments

Comments

Projects
None yet
4 participants
@Jose-Moreno
Copy link
Member

Jose-Moreno commented Oct 5, 2018

Issue Summary

Basically I noticed that If you open a file, and exit the application, the next time you open it, the recent files order will be inverted, and it will continue to shift everytime you open a file (ascending vs descending and viceversa)

Expected Results

Well normally a user should expect always that their working files order are the same unless changed. I I think it's most natural that the last opened files go from top to bottom, being the bottom one the least recent.

Steps to reproduce

Pencil2D starting state
Open recent files are ordered by last opened state

image

  1. I'll open the last one opened (first one in list) "BUG_VECTOR_DRAWING....."

  2. Opening a file does not change this immediately.
    image

  3. Restart the software.

  4. Go to Open recent and notice the list is inverted, the last opened file is still first but the other files are not in the same order.
    image

  5. Do it again, and the order will change again.

System Information

  • Pencil2D Version:
    Version: 0.6.2
    commit: ecbe853
    date: 2018-10-05_13:12:56
    Development build
    Operating System: Windows 7 SP 1 (6.1)
    CPU Architecture: x86_64

@Jose-Moreno Jose-Moreno added bug ux labels Oct 5, 2018

@chchwy chchwy added the hacktoberfest label Oct 6, 2018

@Jose-Moreno Jose-Moreno added this to the hacktoberfest milestone Oct 12, 2018

@scribblemaniac scribblemaniac removed this from the hacktoberfest milestone Nov 3, 2018

@MatthewMcGonagle

This comment has been minimized.

Copy link
Contributor

MatthewMcGonagle commented Dec 19, 2018

I'm interested in trying to fix this. Looking inside core_lib/src/interface/recentfilemenu.cpp, it looks like RecentFileMenu::setRecentFiles() goes through a list of file names in the order of first to last. That is, it uses

    for( QString filename : filenames )
    {
        if ( filename != "" ) {
            addRecentFile( filename );
        }
    }

It calls RecentFileMenu::addRecentFile() which prepends the file name to mRecentFiles; i.e. addRecentFile() calls mRecentFiles.prepend( filename ). So the names will be added to mRecentFiles in the opposite order.

@Jose-Moreno

This comment has been minimized.

Copy link
Member Author

Jose-Moreno commented Dec 19, 2018

@MatthewMcGonagle Sure thing! thank you for your interest in our project. If you need anything let us know 🙂

@MatthewMcGonagle

This comment has been minimized.

Copy link
Contributor

MatthewMcGonagle commented Dec 19, 2018

@Jose-Moreno I think a simple solution would be to just traverse the order of filenames in the opposite order inside setRecentFiles(). So something like:

void RecentFileMenu::setRecentFiles( QStringList filenames )
{

    clear();

    if ( filenames.empty() )
        return;

    QStringList::iterator filename = filenames.end(); 

    do
    {
        filename--;
        if ( *filename != "" ) {
            addRecentFile( *filename );
        }
    } while ( filename != filenames.begin() );
}

This is kind of ugly using an iterator and a while loop. I don't see any way to make reverse iterators for a QList.

Do you know of any way to do this nicely with Qt?

@Jose-Moreno

This comment has been minimized.

Copy link
Member Author

Jose-Moreno commented Dec 19, 2018

@MatthewMcGonagle I'm not sure Qt isn't my forte. I found about this on their documentation tho:

http://doc.qt.io/qt-5/qlist.html#crbegin

Also a related entry in stackoverflow which might help: https://stackoverflow.com/questions/1339121/how-to-reverse-a-qlist

With that said maybe core Pencil2D developers like @scribblemaniac or @chchwy can help you further.

If you want you can also use the #pencil channel on IRC to chat and since that's linked to the discord -#general-development channel as well so your questions will reach the devs promptly so they can assist you with your queries in case it takes too much time to sort it out through github 🙂

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Dec 19, 2018

Thanks @MatthewMcGonagle you make things clear.
Would you like to send a PR?

@MatthewMcGonagle

This comment has been minimized.

Copy link
Contributor

MatthewMcGonagle commented Dec 20, 2018

@Jose-Moreno Thanks for the help. I'll take a look.

@chchwy It's not quite sure to me why the bug results in the last opened file being the in the correct place while the rest of the list is reversed. So I want to take a longer look at it before I submit a PR.

@scribblemaniac

This comment has been minimized.

Copy link
Member

scribblemaniac commented Dec 20, 2018

@MatthewMcGonagle I was also unconvinced that reversing the list was the only issue, so I just dug into it. The reason why the last opened file is in the correct position is because it is being called a second time (after loading) from MainWindow2::readSettings. If you remove that it should work as expected. In fact I think you can remove the LAST_PCLX_PATH setting entirely if you feel up to it.

Adding from oldest to newest when loading a file as you proposed is still necessary. @Jose-Moreno was on the right track for a better way to loop through it in reverse order:

for(auto riter = filenames.crbegin(); riter != filenames.crend(); riter++)
...
@MatthewMcGonagle

This comment has been minimized.

Copy link
Contributor

MatthewMcGonagle commented Dec 20, 2018

@scribblemaniac Thanks for the digging! Okay, I think I see what you are saying. You are referring to the lines

    QString myPath = settings.value(LAST_PCLX_PATH, QVariant(QDir::homePath())).toString();
    mRecentFileMenu->addRecentFile(myPath);

inside MainWindow2::readSettings() in app/src/mainwindows2.cpp. Removing these two lines seems to be okay.

Should they be removed?

I'm not sure if I'm comfortable enough with the code base to remove the LAST_PCLX_PATH setting; I see it is being used by other member functions of MainWindow2.

@scribblemaniac

This comment has been minimized.

Copy link
Member

scribblemaniac commented Jan 5, 2019

@MatthewMcGonagle Sorry, I totally forgot to reply to your message until now. Yes I believe those were the lines I was talking about. If LAST_PCLX_PATH is still being used by other methods, then you can just leave it in, that's no big deal. If it is unnecessary we can remove it later.

@MatthewMcGonagle

This comment has been minimized.

Copy link
Contributor

MatthewMcGonagle commented Jan 8, 2019

@scribblemaniac Thanks for getting back to me, especially after a busy time of year. I'll take a look at it, but probably won't get around to submitting a pull request until next week.

@MatthewMcGonagle

This comment has been minimized.

Copy link
Contributor

MatthewMcGonagle commented Jan 21, 2019

@scribblemaniac I've submitted a pull request. I didn't remove LAST_PCLX_PATH as it is used by other methods in app/src/mainwindows2.cpp, and I can't tell if how these methods are using it is critical to other parts of the application. So I thought it was best to play it safe and leave it alone.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Feb 19, 2019

I am closing this issue as it has been resolved. Thank you @MatthewMcGonagle!

@chchwy chchwy closed this Feb 19, 2019

@chchwy chchwy added this to the 0.6.3 milestone Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.