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

Fixing outlineView.py to enable the outline to show full Title #557

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

BlackXanthus
Copy link

…the name of Character is too long

If the name of a character in POV is a long one, it reduces the amount of space for Title.

This modification (a simple change to the view) ensures that all columns fit it's content. This does enable a scroll-bar at the bottom of the screen for times when the full outline information is too long (QT default behaviour)

@BlackXanthus
Copy link
Author

hmm.... It seems that the build on Semaphore fails because Semaphore doesn't have Qt installed, am I reading that correctly?

@gedakc
Copy link
Collaborator

gedakc commented Apr 23, 2019

Thanks @BlackXanthus for contributing to Manuskript.

The Semaphore Continuous Integration tests are only modifiable by the project owner. As such any changes to these would require action by @olivierkes. In the meantime please ignore issues reported by Semaphore CI.

As a general rule it is best to try to match the coding style used by the project.

Before I review this PR, would you please perform some cleanup on it?

  1. Remove unnecessary whitespace changes. For example do not remove the space between self, and parent:
    QTreeView.__init__(self, parent)

  2. Remove comments referencing old code line(s) and new code line(s).
    The differences are readily available from the git repository using commands like git diff and gitk --all.
    See your initially proposed commit Fixing outlineView.py to enable the outline to show full Title.

  3. Remove mention of filename outlineView.py from the commit title.
    The file name is readily available from the git repository. In the title describe what the commit does in less than 80 characters. Extra explanation can be placed in the body of the commit message.

@BlackXanthus
Copy link
Author

Due to holiday and work, I'll get to this asap.

@BlackXanthus
Copy link
Author

Okay, I think I've caught all the changes that were asked for.

Hopefully I've managed to update it properly.

Thanks,

~BX

@gedakc
Copy link
Collaborator

gedakc commented May 4, 2019

Please squash all of these commits into one. This will make is easier to both review the code and to troubleshoot problems in the future. You will likely need to use the "-f" option when you push the updates to git.

@gedakc
Copy link
Collaborator

gedakc commented May 16, 2019

Again, please squash all of these commits into one.

@BlackXanthus
Copy link
Author

Okay, somehow, my small commit has got wrapped up with someone making a spelling change!

@gedakc
Copy link
Collaborator

gedakc commented Aug 17, 2019

Thanks for trying again. Unfortunately I still see three commits, one of which is the "Fix typo". It would be advisable to rebase against the current develop branch to clean up this issue.

Additionally I still see unnecessary whitespace changes. For example in this change three extra blank lines are added, and a space is removed from QTreeView.__init__(self, parent):

---------------------- manuskript/ui/views/outlineView.py ----------------------
index 95cc786..3479485 100644
@@ -11,8 +11,11 @@ from manuskript.ui.views.outlineDelegates import outlineTitleDelegate, outlineCh
 
 
 class outlineView(QTreeView, dndView, outlineBasics):
+
+	
+
     def __init__(self, parent=None, modelCharacters=None, modelLabels=None, modelStatus=None):
-        QTreeView.__init__(self, parent)
+        QTreeView.__init__(self,parent)
         dndView.__init__(self)
         outlineBasics.__init__(self, parent)

Perhaps it would be simpler to start with a new branch based off the current olivierkes develop branch and only apply the minimum changes (no whitespace changes).

@worstje
Copy link
Contributor

worstje commented Sep 18, 2019

@gedakc I helped him out to fix this PR up for inclusion in 0.10.0; it is ready for merging now. 👍

@gedakc
Copy link
Collaborator

gedakc commented Sep 18, 2019

@worstje how were you able to push changes to the BlackXanthus repo for this PR?

If you could outline the steps, or point me to documentation on how you accomplished this then that would help me in the future.

Thanks,
Curtis

@worstje
Copy link
Contributor

worstje commented Sep 18, 2019

@gedakc BlackXanthus and I are on a Discord server together, so I asked him to invite me to be a collaborator for his repository.

I think you would have had direct access as a maintainer assuming BlackXanthus had checked the box on the PR that lets maintainers push changes to it, but I am not sure about that one as my experience with PRs is almost non-existant on the receiving end. 😄

@gedakc
Copy link
Collaborator

gedakc commented Sep 18, 2019

Thanks for the link.

I'm testing this change and discovered that issues can arise if the chapter or scene name is really long.

Manuskript with this PR applied:
(Note only one column shown and a portion at that - much scrolling required)
Manuskript-Outline-with-PR557

Manuskript without this PR applied:
(Note that "stretch value" makes more columns visible)
Manuskript-Outline-without-PR557

I know there are also reverse issues if other columns have really long names because these force a "stretch" column to be very small in width.

Perhaps a better compromise can be found?

@worstje
Copy link
Contributor

worstje commented Sep 18, 2019

I didn't test this PR; I only cleaned it up. I assumed all the usability testing and such had been covered already.

Personally I prefer the old behaviour, but that is mostly because I have some very long scene names and a big enough screen to get the gist to begin with.

Maybe we'd need a toggle so users can easily switch between whatever mode they prefer?

@gedakc
Copy link
Collaborator

gedakc commented Sep 18, 2019

I also prefer the old behaviour as well.

Currently users can work around the issue by enabling or disabling columns in the Outline view by using Edit -> Settings ->Views -> Outline.

Manuskript-Settings-View-Outline

If a minimum size could be set on the Title column then that might satisfy both situations. However from looking at the QHeaderView Class it appears that the minimumSectionSize applies to all columns. At least that is my interpretation.

EDIT: 'Sorry I missed answering your question.

Maybe we'd need a toggle so users can easily switch between whatever mode they prefer?

Ideally I'd like to follow the K.I.S.S philosophy and not add another control.

@TheJackiMonster
Copy link
Collaborator

As far as I understand this PR, the problem is that either title or POV can take too much space cutting off each other because they both can have pretty long names assigned (they are fully customizable).

I would suggest that the view containing the full table gets a horizontal scroll bar and columns get resizable because this problem can't really be solved automatically. Any author could have different priorities to see title or POV fully.

We can also make it configurable to use automatic fitting sizes for the columns (pretty much the current state) for all users who don't need that customization.

For all users with small screens who still don't want to scroll too much, we can add an overlay on hovering over the names.

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.

4 participants