search

olivierkes / manuskript

A open-source tool for writers
http://www.theologeek.ch/manuskript
GNU General Public License v3.0
1.71k stars 226 forks source link

Pull request with too many things (again, again) #1181

Closed TheShadowOfHassen closed 11 months ago

TheShadowOfHassen commented 1 year ago

I decided to do this on a new branch and new PR because it was easier.

This is the same PR as #1113

TheJackiMonster commented 1 year ago

This is definitely much better to review and merge now. For example the changed files dropped from 31 to 12 and lines of code deleted/changed (in existing code) from 428 to only 28.

I hope it makes sense why I wanted this to be clean? ^^'

The PR is now actually only adding features instead of changing everything.

TheShadowOfHassen commented 1 year ago

So do you want to fix the conflicts or should I?

TheJackiMonster commented 1 year ago

So do you want to fix the conflicts or should I?

I'll fix the conflicts. Don't worry about it.

TheJackiMonster commented 1 year ago

image

The information overlays spawn on the left side of the application window. I think the anchor point needs to be set properly. Would be good to correct this.

Also I don't really like the alignment of the elements. Buttons should all be aligned in a column on the right, I think.

image

Then the names can already cause user error. So either the names should be limited or better the labels could use ellipsis and show the actual name on hover as proper overlay (in case template names are changed externally without respecting a hardcoded limit).

image

The icon of replacing the template should be adjusted as well. I recommend using the icons from Adwaita (best to use symbolic icons) which is typically a GTK default and other themes will adjust from it. You can use the GTK icon browser application to lookup them easily to pick one.

TheShadowOfHassen commented 1 year ago

I'll make the changes as soon as I can.

TheShadowOfHassen commented 1 year ago

I changed the alighments of the buttons and the icon. Was there anything else.

TheShadowOfHassen commented 11 months ago

Yay! Thanks!

TheShadowOfHassen commented 11 months ago

@TheJackiMonster I realized that because this PR had do many things in it it also had a fix for the character more info that would force them to use unique names for the first part because you use dictionaries to store the more info. Does that need merged to the gtk normal? Because without it, if you create things with duplicate names the data in the UI is not what is being stored.

TheJackiMonster commented 11 months ago

For now I'll go with the same behavior as in the Qt version. So I think it would need to be addressed differently. In the Qt version you can actually store multiple values under the same name in the character detailed info.

TheShadowOfHassen commented 11 months ago

OK, just this PR changed that so if you have something else in mind it'll have to be changed back (eventually)