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

11 adjust minimap size #48

Merged
merged 13 commits into from
Jul 18, 2024
Merged

11 adjust minimap size #48

merged 13 commits into from
Jul 18, 2024

Conversation

jakobjo
Copy link
Collaborator

@jakobjo jakobjo commented Jul 11, 2024

Resolved #11

Adjustable minimap

In this implementation, I added an option to in- or decrease the minimap window size. This is done via a context menu containing a slider, that shows upon right clicking inside the minimap.

Changes

  • added Parameter self.minimapsize and setted its default to 400 (only when called without image)
  • added Parameter self.minimumSize and self.maximumSize which can be exchanged with the users desired amount.
  • overrided mousePressEvent() in MinimapWindow(QDialog) class.
    • checks what mouse button was pressed
    • shows custom context menu
  • added createSlider() method, which creates the custom context menu including the slider and an information label.
  • added sliderValueChanged() method
    • is connected to the new slider of createSlider()
    • changes the minimap size according to the slider value

Showcase

default size:

grafik

large minimap:

grafik

small minimap:

grafik

Notes

The min/max size is fixed at min=200 and max=596 but it can still be increased on request. Since the aspect ratio cannot be mantained that easily when changing the window size by just dragging the border, this is the best solution i could think of to still make the minimaps size adjustable for the user.

Requested changes

As requested, the slider now also shows its correct value upon opening the minimap. Also I placed a little hint in the dialog title, so the User can easily find out how to resize the minimap. The size limits have been increased and can now be easily edited if necessary.

Hint is more precise and removes itself after first use of the slider.

@mariehartung mariehartung self-assigned this Jul 11, 2024
@jakobjo jakobjo self-assigned this Jul 11, 2024
@jakobjo jakobjo added this to the Minimap milestone Jul 11, 2024
Copy link
Collaborator

@mariehartung mariehartung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main Review
Sehr schöne Implementation, gute Arbeit!
Funktioniert bei mir (Windows) sehr gut, sowohl mit png, als auch jpg, als auch tif (auch mit großen Testbildern).

Mögliche Änderungen
Bei der leeren Minimap gibt es den (bedienbaren) Slider ja bei der jetzigen Implementation auch noch, er hat aber keinen Effekt; das stört mich jetzt nicht sehr, aber falls es nicht zu aufwendig ist, den Slider da zu entfernen oder auch die leere Minimap dynamisch zu machen, wäre das noch ganz schön.
Wo ich generell ein bisschen ein Problem sehe: Ich denke, dass viele, denen man das nicht explizit sagt, die Funktion gar nicht bemerken werden. Viele würden denke ich einfach versuchen, das Fenster größer zu ziehen, und dann davon ausgehen, dass man die Größe nicht anpassen kann, wenn das nicht funktioniert. Ich habe dafür jetzt auch keinen easy fix, vielleicht kann man das auch einfach in den Fenstertitel schreiben oder dergleichen. Ich würde da aber dir als Product Owner die Entscheidung überlassen, du kannst denke ich besser einschätzen, womit die Kund*innen rechnen und umgehen können.

Nitpicks
Kommentare. Ich habe an den einzelnen Stellen kommentiert, wie ich das grob ergänzen würde, damit der Code nochmal etwas verständlicher wird (auch wenn das davor auch schon gut war).
Die Minimalgröße scheint mir persönlich etwas groß (s. Kommentar).

cellpose/gui/guiparts.py Outdated Show resolved Hide resolved
cellpose/gui/guiparts.py Show resolved Hide resolved
cellpose/gui/guiparts.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@RukiyyeE RukiyyeE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gute Arbeit! Ich finde die Lösung mit dem verstellbaren Slider echt gut umgesetzt. Ich habe jetzt die Größenanpassung mit allen möglichen Bildern, die wir haben, ausprobiert, und alles funktioniert. Auch die Marker, color channels und Sonstiges werden beim Anpassen der Größe der Minimap nicht beeinflusst und bleiben erhalten.

Ich hätte nur ein paar kleine Anmerkungen:

  • Ohne das PR wäre ich nicht darauf gekommen, dass man mit einem Rechtsklick einen Slider erscheinen lassen kann. Vielleicht könnte man irgendwie darauf hinweisen, damit man nicht erst danach suchen muss.
  • Ich weiß nicht, ob das nur bei mir so ist, aber der initiale Slider-Stand passt nicht ganz zur anfänglichen Minimap-Größe:
Bildschirmfoto 2024-07-14 um 23 42 44 Bildschirmfoto 2024-07-14 um 23 43 39

Das könnte man wenn möglich noch anpassen, wäre aber auch nicht schlimm wenn das nicht klappt.

@jakobjo
Copy link
Collaborator Author

jakobjo commented Jul 16, 2024

Hi, thank you for your reviews, I implemented the requested changes, the hint on how to resize is not in the dialog title, i didnt find any other easy solution yet, this could still be added later on, if needed.

@jakobjo jakobjo marked this pull request as ready for review July 16, 2024 13:37
mariehartung
mariehartung previously approved these changes Jul 16, 2024
Copy link
Collaborator

@mariehartung mariehartung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vielen Dank für das schnelle Umsetzen der Reviews!
Super, dass du die Größenvariablen jetzt so übersichtlich gestaltet hast!

Mini-Verbesserungsvorschlag

Würde den Title noch etwas genauer formulieren, sonst bin ich zufrieden.
Ein Triggern der Funktion bei Klick auf Ecken oder dergleichen wäre natürlich noch super, muss aber meiner Meinung nach aber nicht unbedingt sein (und auch nicht in dem issue).

Daher würde ich das jetzt auf jeden Fall mal approven :)

cellpose/gui/guiparts.py Outdated Show resolved Hide resolved
cellpose/gui/guiparts.py Outdated Show resolved Hide resolved
RukiyyeE
RukiyyeE previously approved these changes Jul 17, 2024
Copy link
Collaborator

@RukiyyeE RukiyyeE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sieht gut aus!
Der Slider-Stand und die Minimop-Größe anfangs passen. Voll gut, dass es so schnell geklappt hat. Ich hätte keine Änderungsvorschläge mehr und approve! (Stimme aber Marie zu, dass man den Title genauer formulieren könnte)

Copy link
Collaborator

@NicolasDelgadoL NicolasDelgadoL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the idea of using a slider to solve the resize problem very good. As you mentioned, maintaining the image proportions when dragging the corner had become a challenge that is now circumvented smartly. The implementation is seamless, the slider conveniently appears directly besides the cursor when double clicking, which makes the resizing much more efficient then if one had to navigate to the corner. The slider disappears again with a click on any other part of the screen, which is also effortless and very practical.

As was suggested before, I would also think that the title of the window could be reformulated, so that it informs the user more specifically about the resize functionality. Upon first use, it is not obvious that the double click gives access to the slider.

Other than that, I would also not have any other suggestions, great gob :) !

@jakobjo jakobjo dismissed stale reviews from NicolasDelgadoL, RukiyyeE, and mariehartung via f087814 July 18, 2024 16:19
@jakobjo
Copy link
Collaborator Author

jakobjo commented Jul 18, 2024

I made the hint more precise. The hint is hidden after first slider use.

Copy link
Collaborator

@mariehartung mariehartung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval

Thank you for changing the title!
I have nothing to add, feel free to merge!

Copy link
Collaborator

@RukiyyeE RukiyyeE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great solution! I approve

@jakobjo jakobjo merged commit 3bf7ce3 into main Jul 18, 2024
2 checks passed
@jakobjo jakobjo deleted the 11_adjust_minimap_size branch July 18, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adjust minimap size epic minimap (2)
6 participants