JJ: Add in author profiles to things that are tracked by sessions.

Authored by albertoefg on Oct 12 2018, 3:24 PM.



Added code on /libs/ui/KisSessionResource.cpp
Now it saves the session with the authorProfile

Changed /libs/ui/KisViewManager.h

Now the following functions are public:

void changeAuthorProfile(const QString &profileName);
void slotUpdateAuthorProfileActions();

Test Plan

I compiled it, and it works, loads the session with the profile selected.

Diff Detail

R37 Krita
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
albertoefg created this revision.Oct 12 2018, 3:24 PM
Restricted Application added a project: Krita. · View Herald TranscriptOct 12 2018, 3:24 PM
albertoefg requested review of this revision.Oct 12 2018, 3:24 PM
albertoefg edited the summary of this revision. (Show Details)
jounip accepted this revision.Oct 12 2018, 6:31 PM
jounip added a subscriber: jounip.

Nice work.

It seems to work as advertised. I commented a couple of little nitpicks (see the inline comments), but overall, the patch looks good to me.

This revision is now accepted and ready to land.Oct 12 2018, 6:31 PM
jounip added inline comments.Oct 12 2018, 6:34 PM

Preferably that this method would use the values already in the session object, in this case d->profileName.
The value should be fetched into d->profileName in storeCurrentWindows (which, in hindsight, I should have named something like storeCurrentState in the first place).


Ideally, this would be done outside the loop, but since the view manager isn't readily available otherwise, it's fine. We should probably to refactor the changeAuthorProfile function to a more sensible place.
As a sidenote, you could pass profileName directly to the function like this: window->viewManager()->changeAuthorProfile(d->profileName);

albertoefg added inline comments.Oct 13 2018, 2:09 PM

Hi, thanks for reviewing my code :)
Honestly I did not think about that because this was the first part of the code that I wrote.

However I think we should keep it this way, because if it were read from d->profileName, there will be no way to set or change the author, as it will always be read from d->profileName.

The other thing I think we could do, will be to modify the way changeAuthorProfile() works on the viewManager, to also read the XML and delete the current profileName and set a new one, but I think that will create too much code and work, as it will need to modify and save the XML.

You mention a refactor so the second option might indeed be the best one. Please let me know what you think so I can make the proper changes.


Yes, sorry about that! I was trying to be very sure that it was working so I was more verbose than I probably should. Should I change it and send it?

I am sorry this is my first patch ever to any project so I am not completely sure how this works :)

jounip added inline comments.Oct 13 2018, 3:31 PM

The storeCurrentWindows function is called when a session is closed, so the author would be updated just like the windows and documents are now.

The two functions are called one after another, so functionally it makes little difference. I would just prefer to organize related operations together (gathering the data that makes a session vs. serializing it into xml).


No need to apologize. The important thing is that the code works and isn't an unreadable mess :)

Your patch is perfectly fine on both accounts. The rest is more about tidying up and polishing. As you get more familiar with the language and the code, I'm sure you'll pick up the coding style we try to follow.

Also, it looks like I posted this comment in the wrong place. It was meant to be for line 100. Sorry about any confusion that may have caused.

This revision was automatically updated to reflect the committed changes.